Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1567783005:
Run ac3, eac3 and hevc unit tests in all configs (Closed)
|
Created:
4 years, 11 months ago by servolk Modified:
4 years, 10 months ago Reviewers:
ddorwin CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@ac3-eac3-passthrough Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRun ac3, eac3 and hevc unit tests in all configs
Previously we would run ac3, eac3 and hevc media unit tests only on
configurations where those flags were set (effectively only Chromecast/
cast_shell for now). With this change we will run those tests on all
configs and will expect the data parsing to fail gracefully on builds
where corresponding formats/codecs are not supported.
Committed: https://crrev.com/bd30c50c7f4d17a1ae30f42f9848e08c7bb31d80
Cr-Commit-Position: refs/heads/master@{#377168}
Patch Set 1 #Patch Set 2 : CR feedback #Patch Set 3 : Rebase + convert hevc tests #Patch Set 4 : rebase #Patch Set 5 : rebase #
Total comments: 8
Patch Set 6 : CR feedback #Patch Set 7 : rebase #
Total comments: 16
Patch Set 8 : Refactoring #
Messages
Total messages: 15 (6 generated)
Description was changed from
==========
Run ac3/eac3 unit tests in all configs
BUG=none
==========
to
==========
Run ac3, eac3 and hevc unit tests in all configs
Previously we would run ac3, eac3 and hevc media unit tests only on
configurations where those flags were set (effectively only Chromecast/
cast_shell for now). With this change we will run those tests on all
configs and will expect the data parsing to fail gracefully on builds
where corresponding formats/codecs are not supported.
BUG=none
==========
servolk@chromium.org changed reviewers: + ddorwin@chromium.org
Thanks. You can drop BUG= if there isn't one. Alternatively, you can use the BUGs for which these tests were originally added. https://codereview.chromium.org/1567783005/diff/80001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/1567783005/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer_unittest.cc:103: if (demuxer_init_expected_status_ == PIPELINE_OK) This doesn't really need to be a member. Add InitializeDemuxerExpectUnsupportedFormat() or something like that to pass DEMUXER_ERROR_NO_SUPPORTED_STREAMS to this function as |expected_status|. Since we have two calls anyway (in the #if and #else), this will be clearer https://codereview.chromium.org/1567783005/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer_unittest.cc:111: void InitializeDemuxerText(bool enable_text) { Unrelated except that it may avoid adding an |expected_status| parameter here: Calling this from InitializeDemuxer() seems overly complex. This should probably just be called InitializeDemuxerWithText() then InitializeDemuxer should call the function below directly. That also avoids unclear |true| parameters at the call site. WDYT? https://codereview.chromium.org/1567783005/diff/80001/media/formats/mp4/mp4_s... File media/formats/mp4/mp4_stream_parser_unittest.cc (right): https://codereview.chromium.org/1567783005/diff/80001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser_unittest.cc:308: bool expect_success = true; nit: Might be better for symmetry to put this inside the if. Also, the default (most common) is really false. This is consistent with the others below, but those also use the inverse logic, so I'm not sure how much that matters. Anyway, up to you. https://codereview.chromium.org/1567783005/diff/80001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser_unittest.cc:310: EXPECT_MEDIA_LOG(VideoCodecLog("hevc")); Is there a reason we don't have these for the audio codecs below?
Description was changed from
==========
Run ac3, eac3 and hevc unit tests in all configs
Previously we would run ac3, eac3 and hevc media unit tests only on
configurations where those flags were set (effectively only Chromecast/
cast_shell for now). With this change we will run those tests on all
configs and will expect the data parsing to fail gracefully on builds
where corresponding formats/codecs are not supported.
BUG=none
==========
to
==========
Run ac3, eac3 and hevc unit tests in all configs
Previously we would run ac3, eac3 and hevc media unit tests only on
configurations where those flags were set (effectively only Chromecast/
cast_shell for now). With this change we will run those tests on all
configs and will expect the data parsing to fail gracefully on builds
where corresponding formats/codecs are not supported.
==========
https://codereview.chromium.org/1567783005/diff/80001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/1567783005/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer_unittest.cc:103: if (demuxer_init_expected_status_ == PIPELINE_OK) On 2016/01/23 00:38:56, ddorwin wrote: > This doesn't really need to be a member. > > Add InitializeDemuxerExpectUnsupportedFormat() or something like that to pass > DEMUXER_ERROR_NO_SUPPORTED_STREAMS to this function as |expected_status|. Since > we have two calls anyway (in the #if and #else), this will be clearer Done. https://codereview.chromium.org/1567783005/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer_unittest.cc:111: void InitializeDemuxerText(bool enable_text) { On 2016/01/23 00:38:56, ddorwin wrote: > Unrelated except that it may avoid adding an |expected_status| parameter here: > > Calling this from InitializeDemuxer() seems overly complex. This should probably > just be called InitializeDemuxerWithText() then InitializeDemuxer should call > the function below directly. That also avoids unclear |true| parameters at the > call site. > > WDYT? Yeah, I think it would be cleaner if we just had a single InitializeDemuxer method with 3 parameters, and then we can use default arguments to keep usage simple for most common cases. https://codereview.chromium.org/1567783005/diff/80001/media/formats/mp4/mp4_s... File media/formats/mp4/mp4_stream_parser_unittest.cc (right): https://codereview.chromium.org/1567783005/diff/80001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser_unittest.cc:308: bool expect_success = true; On 2016/01/23 00:38:56, ddorwin wrote: > nit: Might be better for symmetry to put this inside the if. Also, the default > (most common) is really false. > > This is consistent with the others below, but those also use the inverse logic, > so I'm not sure how much that matters. Anyway, up to you. Done (fixed also the inverse logic below for consistency). https://codereview.chromium.org/1567783005/diff/80001/media/formats/mp4/mp4_s... media/formats/mp4/mp4_stream_parser_unittest.cc:310: EXPECT_MEDIA_LOG(VideoCodecLog("hevc")); On 2016/01/23 00:38:56, ddorwin wrote: > Is there a reason we don't have these for the audio codecs below? In case of HEVC we do some trivial parsing of the bit stream (we parse the hvcC box in the mp4 file, that contains the HEVCDecoderConfigurationRecord structure, since we want to know the video size/resolution), and after that parsing is finished successfully the VideoCodec message is logged. In case of AC3/EAC3 audio we support only pass-through, we don't need to know any parameters of the input streams, so we just demux those and never even try to parse them, so no AudioCodec is logged to the MSE media log. We do have audio codec logging in lower level of Chromecast media stack, so it doesn't matter, but if you prefer, I can also add AudioCodec log message for AC3/EAC3.
On 2016/02/10 18:58:50, servolk wrote: > https://codereview.chromium.org/1567783005/diff/80001/media/filters/ffmpeg_de... > File media/filters/ffmpeg_demuxer_unittest.cc (right): > > https://codereview.chromium.org/1567783005/diff/80001/media/filters/ffmpeg_de... > media/filters/ffmpeg_demuxer_unittest.cc:103: if (demuxer_init_expected_status_ > == PIPELINE_OK) > On 2016/01/23 00:38:56, ddorwin wrote: > > This doesn't really need to be a member. > > > > Add InitializeDemuxerExpectUnsupportedFormat() or something like that to pass > > DEMUXER_ERROR_NO_SUPPORTED_STREAMS to this function as |expected_status|. > Since > > we have two calls anyway (in the #if and #else), this will be clearer > > Done. > > https://codereview.chromium.org/1567783005/diff/80001/media/filters/ffmpeg_de... > media/filters/ffmpeg_demuxer_unittest.cc:111: void InitializeDemuxerText(bool > enable_text) { > On 2016/01/23 00:38:56, ddorwin wrote: > > Unrelated except that it may avoid adding an |expected_status| parameter here: > > > > Calling this from InitializeDemuxer() seems overly complex. This should > probably > > just be called InitializeDemuxerWithText() then InitializeDemuxer should call > > the function below directly. That also avoids unclear |true| parameters at the > > call site. > > > > WDYT? > > Yeah, I think it would be cleaner if we just had a single InitializeDemuxer > method with 3 parameters, and then we can use default arguments to keep usage > simple for most common cases. > > https://codereview.chromium.org/1567783005/diff/80001/media/formats/mp4/mp4_s... > File media/formats/mp4/mp4_stream_parser_unittest.cc (right): > > https://codereview.chromium.org/1567783005/diff/80001/media/formats/mp4/mp4_s... > media/formats/mp4/mp4_stream_parser_unittest.cc:308: bool expect_success = true; > On 2016/01/23 00:38:56, ddorwin wrote: > > nit: Might be better for symmetry to put this inside the if. Also, the default > > (most common) is really false. > > > > This is consistent with the others below, but those also use the inverse > logic, > > so I'm not sure how much that matters. Anyway, up to you. > > Done (fixed also the inverse logic below for consistency). > > https://codereview.chromium.org/1567783005/diff/80001/media/formats/mp4/mp4_s... > media/formats/mp4/mp4_stream_parser_unittest.cc:310: > EXPECT_MEDIA_LOG(VideoCodecLog("hevc")); > On 2016/01/23 00:38:56, ddorwin wrote: > > Is there a reason we don't have these for the audio codecs below? > > In case of HEVC we do some trivial parsing of the bit stream (we parse the hvcC > box in the mp4 file, that contains the HEVCDecoderConfigurationRecord structure, > since we want to know the video size/resolution), and after that parsing is > finished successfully the VideoCodec message is logged. In case of AC3/EAC3 > audio we support only pass-through, we don't need to know any parameters of the > input streams, so we just demux those and never even try to parse them, so no > AudioCodec is logged to the MSE media log. We do have audio codec logging in > lower level of Chromecast media stack, so it doesn't matter, but if you prefer, > I can also add AudioCodec log message for AC3/EAC3. ping
https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:102: bool enable_text = false, bools are always a problem because there is no context for what it means at the call site. I see you have commented on the meaning in some cases, which is helpful. https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:103: media::PipelineStatus demuxer_init_expected_status = PIPELINE_OK, Looking at the uses and considering the bool issue above and potential issues with default parameters, it seems that separate functions would be more readable, potentially with less text. Specific suggestions below. Then this would be a private function without default parameters that each of those call. https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:355: InitializeDemuxer(/*enable_text=*/true); InitializeDemuxerWithText(); https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:450: InitializeDemuxer(false, PIPELINE_OK, InitializeDemuxer(base::Time::FromJsTime(kTimelineOffsetMs)); InitializeDemuxer always expects success and never enables text. https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:1127: InitializeDemuxer(false, DEMUXER_ERROR_NO_SUPPORTED_STREAMS); InitializeDemuxerAndExpectUnsupportedFormat() OR InitializeDemuxerAndExpectError(DEMUXER_ERROR_NO_SUPPORTED_STREAMS) https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:1169: #endif Is there a reason not to run these tests in all cases? Do they fail differently? (Ideally, we'd do the same thing as this CL to the tests before 1112.) https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:1169: #endif // defined(USE_PROPRIETARY_CODECS) This is a long block, so we should include it in this case. I know it wasn't there before. :)
https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:102: bool enable_text = false, On 2016/02/19 20:51:10, ddorwin wrote: > bools are always a problem because there is no context for what it means at the > call site. I see you have commented on the meaning in some cases, which is > helpful. Acknowledged. https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:103: media::PipelineStatus demuxer_init_expected_status = PIPELINE_OK, On 2016/02/19 20:51:10, ddorwin wrote: > Looking at the uses and considering the bool issue above and potential issues > with default parameters, it seems that separate functions would be more > readable, potentially with less text. Specific suggestions below. > > Then this would be a private function without default parameters that each of > those call. Done. https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:355: InitializeDemuxer(/*enable_text=*/true); On 2016/02/19 20:51:09, ddorwin wrote: > InitializeDemuxerWithText(); Done. https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:450: InitializeDemuxer(false, PIPELINE_OK, On 2016/02/19 20:51:10, ddorwin wrote: > InitializeDemuxer(base::Time::FromJsTime(kTimelineOffsetMs)); > > InitializeDemuxer always expects success and never enables text. Given that the timeline_offset parameter is used/needed by only one test case, I'd rather have a separate InitializeDemuxerWithTimelineOffset overload, to keep all the other places simple with parameterless invocations of InitDemuxer. WDYT? https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:1127: InitializeDemuxer(false, DEMUXER_ERROR_NO_SUPPORTED_STREAMS); On 2016/02/19 20:51:09, ddorwin wrote: > InitializeDemuxerAndExpectUnsupportedFormat() > OR InitializeDemuxerAndExpectError(DEMUXER_ERROR_NO_SUPPORTED_STREAMS) Done. https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:1169: #endif On 2016/02/19 20:51:09, ddorwin wrote: > // defined(USE_PROPRIETARY_CODECS) > > This is a long block, so we should include it in this case. I know it wasn't > there before. :) Done. https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:1169: #endif On 2016/02/19 20:51:10, ddorwin wrote: > Is there a reason not to run these tests in all cases? Do they fail differently? > (Ideally, we'd do the same thing as this CL to the tests before 1112.) Well, some earlier patchsets of this CL (e.g. PS #5) placed these tests outside of the USE_PROPRIETARY_CODECS section. This failed on some trybots, e.g. linux_chromium_chromeos_asan_rel_ng https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... (other trybot failure results got deleted already, but they all looked like this one). From the failure logs it looks like proprietary media files from media/test/data were not present (since we got PipelineStatus == 12 == DEMUXER_ERROR_COULD_NOT_OPEN, instead of expected 14 == DEMUXER_ERROR_NO_SUPPORTED_STREAMS). So if we want to enable these tests on all trybots, first we'll need to ensure that test files with proprietary codecs are present on all trybots, even on those that build with USE_PROPRIETARY_CODECS=false. I'm not sure if we want to do that.
lgtm Thanks! https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:450: InitializeDemuxer(false, PIPELINE_OK, On 2016/02/19 21:32:01, servolk wrote: > On 2016/02/19 20:51:10, ddorwin wrote: > > InitializeDemuxer(base::Time::FromJsTime(kTimelineOffsetMs)); > > > > InitializeDemuxer always expects success and never enables text. > > Given that the timeline_offset parameter is used/needed by only one test case, > I'd rather have a separate InitializeDemuxerWithTimelineOffset overload, to keep > all the other places simple with parameterless invocations of InitDemuxer. WDYT? Acknowledged. https://codereview.chromium.org/1567783005/diff/120001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer_unittest.cc:1169: #endif On 2016/02/19 21:32:01, servolk wrote: > On 2016/02/19 20:51:10, ddorwin wrote: > > Is there a reason not to run these tests in all cases? Do they fail > differently? > > (Ideally, we'd do the same thing as this CL to the tests before 1112.) > > Well, some earlier patchsets of this CL (e.g. PS #5) placed these tests outside > of the USE_PROPRIETARY_CODECS section. This failed on some trybots, e.g. > linux_chromium_chromeos_asan_rel_ng > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... > (other trybot failure results got deleted already, but they all looked like this > one). From the failure logs it looks like proprietary media files from > media/test/data were not present (since we got PipelineStatus == 12 == > DEMUXER_ERROR_COULD_NOT_OPEN, instead of expected 14 == > DEMUXER_ERROR_NO_SUPPORTED_STREAMS). So if we want to enable these tests on all > trybots, first we'll need to ensure that test files with proprietary codecs are > present on all trybots, even on those that build with > USE_PROPRIETARY_CODECS=false. I'm not sure if we want to do that. Acknowledged.
The CQ bit was checked by servolk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1567783005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567783005/140001
Message was sent while issue was closed.
Description was changed from
==========
Run ac3, eac3 and hevc unit tests in all configs
Previously we would run ac3, eac3 and hevc media unit tests only on
configurations where those flags were set (effectively only Chromecast/
cast_shell for now). With this change we will run those tests on all
configs and will expect the data parsing to fail gracefully on builds
where corresponding formats/codecs are not supported.
==========
to
==========
Run ac3, eac3 and hevc unit tests in all configs
Previously we would run ac3, eac3 and hevc media unit tests only on
configurations where those flags were set (effectively only Chromecast/
cast_shell for now). With this change we will run those tests on all
configs and will expect the data parsing to fail gracefully on builds
where corresponding formats/codecs are not supported.
==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from
==========
Run ac3, eac3 and hevc unit tests in all configs
Previously we would run ac3, eac3 and hevc media unit tests only on
configurations where those flags were set (effectively only Chromecast/
cast_shell for now). With this change we will run those tests on all
configs and will expect the data parsing to fail gracefully on builds
where corresponding formats/codecs are not supported.
==========
to
==========
Run ac3, eac3 and hevc unit tests in all configs
Previously we would run ac3, eac3 and hevc media unit tests only on
configurations where those flags were set (effectively only Chromecast/
cast_shell for now). With this change we will run those tests on all
configs and will expect the data parsing to fail gracefully on builds
where corresponding formats/codecs are not supported.
Committed: https://crrev.com/bd30c50c7f4d17a1ae30f42f9848e08c7bb31d80
Cr-Commit-Position: refs/heads/master@{#377168}
==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/bd30c50c7f4d17a1ae30f42f9848e08c7bb31d80 Cr-Commit-Position: refs/heads/master@{#377168} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
