Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1581003002:
MSE: Strictly verify expected MediaLog events in ChunkDemuxerTests (Closed)
DescriptionMSE: Strictly verify expected MediaLog events in ChunkDemuxerTests
This CL continues to increase strict media unit test coverage of
expected media-internals logs emitted during tests. In particular, this
CL adds strict MediaLog event verification to ChunkDemuxerTests.
It includes a few new test cases, and refactors many of the previous
single-stream cluster appends to muxed A/V SourceBuffers to use
more well-formed muxed cluster appends.
R=chcunningham@chromium.org,xhwang@chromium.org
TEST=ChunkDemuxerTests
BUG=524597
Committed: https://crrev.com/cd0b970239d9a8067ec832f7a4fca620f890d187
Cr-Commit-Position: refs/heads/master@{#369666}
Patch Set 1 #Patch Set 2 : rebase to ToT #Patch Set 3 : Fix the mpeg2ts parser medialog expectation #
Total comments: 4
Patch Set 4 : Rebase to ToT -- seems like a broken rebase. I'll do another. #Patch Set 5 : Manual application of patch set 3 to ToT #Patch Set 6 : Addressed chcunningham's comment #Messages
Total messages: 21 (10 generated)
Description was changed from ========== MSE: Strictly verify expected MediaLog events in ChunkDemuxerTests This CL continues to increase strict media unit test coverage of expected media-internals logs emitted during tests. In particular, this CL adds strict MediaLog event verification to ChunkDemuxerTests. R=chcunningham@chromium.org TEST=ChunkDemuxerTests BUG=524597 ========== to ========== MSE: Strictly verify expected MediaLog events in ChunkDemuxerTests This CL continues to increase strict media unit test coverage of expected media-internals logs emitted during tests. In particular, this CL adds strict MediaLog event verification to ChunkDemuxerTests. It includes a few new test cases, and refactors many of the previous single-stream cluster appends to muxed A/V SourceBuffers to use more well-formed muxed cluster appends. R=chcunningham@chromium.org TEST=ChunkDemuxerTests BUG=524597 ==========
Description was changed from ========== MSE: Strictly verify expected MediaLog events in ChunkDemuxerTests This CL continues to increase strict media unit test coverage of expected media-internals logs emitted during tests. In particular, this CL adds strict MediaLog event verification to ChunkDemuxerTests. It includes a few new test cases, and refactors many of the previous single-stream cluster appends to muxed A/V SourceBuffers to use more well-formed muxed cluster appends. R=chcunningham@chromium.org TEST=ChunkDemuxerTests BUG=524597 ========== to ========== MSE: Strictly verify expected MediaLog events in ChunkDemuxerTests This CL continues to increase strict media unit test coverage of expected media-internals logs emitted during tests. In particular, this CL adds strict MediaLog event verification to ChunkDemuxerTests. It includes a few new test cases, and refactors many of the previous single-stream cluster appends to muxed A/V SourceBuffers to use more well-formed muxed cluster appends. R=chcunningham@chromium.org TEST=ChunkDemuxerTests BUG=524597 ==========
Description was changed from ========== MSE: Strictly verify expected MediaLog events in ChunkDemuxerTests This CL continues to increase strict media unit test coverage of expected media-internals logs emitted during tests. In particular, this CL adds strict MediaLog event verification to ChunkDemuxerTests. It includes a few new test cases, and refactors many of the previous single-stream cluster appends to muxed A/V SourceBuffers to use more well-formed muxed cluster appends. R=chcunningham@chromium.org TEST=ChunkDemuxerTests BUG=524597 ========== to ========== MSE: Strictly verify expected MediaLog events in ChunkDemuxerTests This CL continues to increase strict media unit test coverage of expected media-internals logs emitted during tests. In particular, this CL adds strict MediaLog event verification to ChunkDemuxerTests. It includes a few new test cases, and refactors many of the previous single-stream cluster appends to muxed A/V SourceBuffers to use more well-formed muxed cluster appends. R=chcunningham@chromium.org,xhwang@chromium.org TEST=ChunkDemuxerTests BUG=524597 ==========
Please take a look: chcunningham@: everything xhwang@: sanity check of relocation of the OnEncryptedMediaData() expectation addition logic in the tests (which allows the InSequence MediaLog expectations of a few related tests to be strictly tested).
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1581003002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1581003002/40001
wolenetz@chromium.org changed reviewers: + xhwang@chromium.org
R+=xhwang@ per above.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://chromiumcodereview.appspot.com/1581003002/diff/40001/media/filters/ch... File media/filters/chunk_demuxer_unittest.cc (right): https://chromiumcodereview.appspot.com/1581003002/diff/40001/media/filters/ch... media/filters/chunk_demuxer_unittest.cc:199: } I was a bit worried about the maintainability of these tests since every time we change where we report these logs, or change the log text message, we need to update these tests. But then on the other side these tests can make sure we always report the correct text message in the right place, which is a good thing :)
On 2016/01/13 19:04:38, xhwang wrote: > lgtm > > https://chromiumcodereview.appspot.com/1581003002/diff/40001/media/filters/ch... > File media/filters/chunk_demuxer_unittest.cc (right): > > https://chromiumcodereview.appspot.com/1581003002/diff/40001/media/filters/ch... > media/filters/chunk_demuxer_unittest.cc:199: } > I was a bit worried about the maintainability of these tests since every time we > change where we report these logs, or change the log text message, we need to > update these tests. But then on the other side these tests can make sure we > always report the correct text message in the right place, which is a good thing > :) I was likewise worried, but also hopeful that clamping down on strictness will help us have more confidence about the logs we emit. There's also currently a little duplication of the macro expectations across chunk_demuxer_unittests.cc and webm_*tests.cc and source_buffer_stream_*tests.cc introduced in this CL. The scale of duplication was small. We don't have a media/filters/filter_test_helpers.{h,cc} like media/base/test_helpers.{h,cc}. Should something like these macro expectations be added there.
lgtm https://codereview.chromium.org/1581003002/diff/40001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (left): https://codereview.chromium.org/1581003002/diff/40001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2561: CheckExpectedRanges("{ [0,23) [320,400) [520,570) [720,750) [920,950) }"); Did you mean to remove this CheckExpectedRanges?
Thank you for reviews. Sending to CQ https://codereview.chromium.org/1581003002/diff/40001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (left): https://codereview.chromium.org/1581003002/diff/40001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:2561: CheckExpectedRanges("{ [0,23) [320,400) [520,570) [720,750) [920,950) }"); On 2016/01/14 19:11:23, chcunningham wrote: > Did you mean to remove this CheckExpectedRanges? Good catch. Fixed. https://codereview.chromium.org/1581003002/diff/40001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1581003002/diff/40001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:199: } On 2016/01/13 19:04:38, xhwang wrote: > I was a bit worried about the maintainability of these tests since every time we > change where we report these logs, or change the log text message, we need to > update these tests. But then on the other side these tests can make sure we > always report the correct text message in the right place, which is a good thing > :) Acknowledged.
The CQ bit was checked by wolenetz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chcunningham@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/1581003002/#ps100001 (title: "Addressed chcunningham's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1581003002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1581003002/100001
Message was sent while issue was closed.
Description was changed from ========== MSE: Strictly verify expected MediaLog events in ChunkDemuxerTests This CL continues to increase strict media unit test coverage of expected media-internals logs emitted during tests. In particular, this CL adds strict MediaLog event verification to ChunkDemuxerTests. It includes a few new test cases, and refactors many of the previous single-stream cluster appends to muxed A/V SourceBuffers to use more well-formed muxed cluster appends. R=chcunningham@chromium.org,xhwang@chromium.org TEST=ChunkDemuxerTests BUG=524597 ========== to ========== MSE: Strictly verify expected MediaLog events in ChunkDemuxerTests This CL continues to increase strict media unit test coverage of expected media-internals logs emitted during tests. In particular, this CL adds strict MediaLog event verification to ChunkDemuxerTests. It includes a few new test cases, and refactors many of the previous single-stream cluster appends to muxed A/V SourceBuffers to use more well-formed muxed cluster appends. R=chcunningham@chromium.org,xhwang@chromium.org TEST=ChunkDemuxerTests BUG=524597 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MSE: Strictly verify expected MediaLog events in ChunkDemuxerTests This CL continues to increase strict media unit test coverage of expected media-internals logs emitted during tests. In particular, this CL adds strict MediaLog event verification to ChunkDemuxerTests. It includes a few new test cases, and refactors many of the previous single-stream cluster appends to muxed A/V SourceBuffers to use more well-formed muxed cluster appends. R=chcunningham@chromium.org,xhwang@chromium.org TEST=ChunkDemuxerTests BUG=524597 ========== to ========== MSE: Strictly verify expected MediaLog events in ChunkDemuxerTests This CL continues to increase strict media unit test coverage of expected media-internals logs emitted during tests. In particular, this CL adds strict MediaLog event verification to ChunkDemuxerTests. It includes a few new test cases, and refactors many of the previous single-stream cluster appends to muxed A/V SourceBuffers to use more well-formed muxed cluster appends. R=chcunningham@chromium.org,xhwang@chromium.org TEST=ChunkDemuxerTests BUG=524597 Committed: https://crrev.com/cd0b970239d9a8067ec832f7a4fca620f890d187 Cr-Commit-Position: refs/heads/master@{#369666} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cd0b970239d9a8067ec832f7a4fca620f890d187 Cr-Commit-Position: refs/heads/master@{#369666} |
