AudioVideoMetadataExtractor reports an integer, converted to double, for duration |
|||
Issue descriptionWhile investigating adding new extractor unittests (to verify .flac and flac.mp4 extraction as part of cleanup work leading to fixing bug 666000 ), I discovered strangely some existing expectations of "0" duration for nonzero duration media files. Looking further, I found that the internal |duration_| type in the extractor is an int [1], though it is populated from a double, and converted back to a double upon retrieval via ::duration(). This bug is to document clearly what / why this is the case (or to fix it if it was unintentionally converted through an integer internal value). [1] https://cs.chromium.org/chromium/src/media/filters/audio_video_metadata_extractor.h?sq=package:chromium&l=80
,
Jul 21 2017
Looks like indeed we should be storing it as a double. Looking at https://www.ffmpeg.org/doxygen/3.0/structAVFormatContext.html#ad0ea78ac48f5bb0a15a15c1c472744d9, the intent was that it starts out as an integer in ffmpeg, and we want to convert it to a double. I'm not sure why we stored it as an integer -- probably so we could return -1 if there was no duration. So as of right now, I'm guessing it effectively applies a floor function to all the durations to the closest integer rounding down.
,
Jul 21 2017
Correct fix may be to add a separate has_duration() bool or something... Is this blocking you on anything?
,
Jul 21 2017
This is not blocking me. I just found it curious and bug-worthy :) I'll be sending out a CL shortly with some new AudioVideoMetadataExtractorTest cases, including some comments detailing the expected "double" value of duration for all the current cases, and referencing this bug. tommycli@, I'll include you as reviewer on it.
,
Jul 21 2017
Or if you want, go ahead and fix it! The fix I think will be simple. Add a boolean has_duration flag, defaulting to false. Change duration_ to a double. In this block: https://cs.chromium.org/chromium/src/media/filters/audio_video_metadata_extractor.cc?sq=package:chromium&l=97 Set has_duration to true. In the places where callers check if duration() >= 0, use the has_duration flag instead. Thanks.
,
Jul 21 2017
Fix is out for review at https://chromium-review.googlesource.com/582287
,
Jul 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/446e5b893436ceab5dae61e87d5df95467348975 commit 446e5b893436ceab5dae61e87d5df95467348975 Author: Matt Wolenetz <wolenetz@chromium.org> Date: Fri Jul 21 21:36:32 2017 Add test cases to verify extracted metadata for .flac and flac.mp4 Since M56, we've had .flac and flac-in-mp4 demux and decode support in Chrome, but we haven't included AudioVideoMetadataExtractorTests to confirm expectations. This change adds those. It also calls out expected "double" values for duration metadata that might be needed when bug 747480 is fixed. BUG= 666000 , 747480 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I072ba8561d0912169848e96deaf87dfc636ee54b Reviewed-on: https://chromium-review.googlesource.com/581937 Reviewed-by: Tommy Li <tommycli@chromium.org> Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org> Cr-Commit-Position: refs/heads/master@{#488754} [modify] https://crrev.com/446e5b893436ceab5dae61e87d5df95467348975/media/filters/audio_video_metadata_extractor_unittest.cc
,
Jul 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b17fc441b2d272d7b1582e6d39a474c5003130d7 commit b17fc441b2d272d7b1582e6d39a474c5003130d7 Author: Matt Wolenetz <wolenetz@chromium.org> Date: Tue Jul 25 19:53:59 2017 Report fractional duration values from AudioVideoMetadataExtractor This change implements the fix suggested in [1], no longer truncating extracted duration values to an integer and then converting to a double, but retaining the original double value throughout. It also explicitly reports whether or not duration information was extracted, and DCHECK-fails if duration information was not extracted but is asked for anyways. [1] https://bugs.chromium.org/p/chromium/issues/detail?id=747480#c5 BUG= 747480 TESTS=Updated extractor tests Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I4ad9be1c7fd6e264bed5c4e2c4a06e0970a65504 Reviewed-on: https://chromium-review.googlesource.com/582287 Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org> Reviewed-by: Tommy Li <tommycli@chromium.org> Cr-Commit-Position: refs/heads/master@{#489405} [modify] https://crrev.com/b17fc441b2d272d7b1582e6d39a474c5003130d7/chrome/utility/media_galleries/media_metadata_parser.cc [modify] https://crrev.com/b17fc441b2d272d7b1582e6d39a474c5003130d7/media/filters/audio_video_metadata_extractor.cc [modify] https://crrev.com/b17fc441b2d272d7b1582e6d39a474c5003130d7/media/filters/audio_video_metadata_extractor.h [modify] https://crrev.com/b17fc441b2d272d7b1582e6d39a474c5003130d7/media/filters/audio_video_metadata_extractor_unittest.cc
,
Jul 25 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by wolenetz@chromium.org
, Jul 21 2017