Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1860263002:
Support alternate audio/video track ids in ChunkDemuxer tests (Closed)
DescriptionSupport alternate audio/video track ids in ChunkDemuxer tests
Also added a new unit test case where audio and video track ids change
in the second init segment, and did some minor refactoring (renamed
InitSegmentReceived -> InitSegmentReceivedMock).
Committed: https://crrev.com/cb32bef64809077450a0e259053e3ef5734a543c
Cr-Commit-Position: refs/heads/master@{#386248}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase + refactoring #Patch Set 4 : rebase #Patch Set 5 : rebase #Patch Set 6 : test fix #Patch Set 7 : rebase #Patch Set 8 : rebase #
Total comments: 8
Patch Set 9 : CR feedback #Messages
Total messages: 20 (10 generated)
Description was changed from ========== Support alternate audio/video track ids in ChunkDemuxer tests Also added a new unit test case where audio and video track ids change in the second init segment. ========== to ========== Support alternate audio/video track ids in ChunkDemuxer tests Also added a new unit test case where audio and video track ids change in the second init segment. ==========
servolk@chromium.org changed reviewers: + chcunningham@chromium.org, wolenetz@chromium.org
PTAL. I'm planning to use these alternate ids for audio and video tracks to extend test coverage for https://codereview.chromium.org/1812543003/
Description was changed from ========== Support alternate audio/video track ids in ChunkDemuxer tests Also added a new unit test case where audio and video track ids change in the second init segment. ========== to ========== Support alternate audio/video track ids in ChunkDemuxer tests Also added a new unit test case where audio and video track ids change in the second init segment, and did some minor refactoring (renamed InitSegmentReceived -> InitSegmentReceivedMock). ==========
On 2016/04/05 23:00:35, servolk wrote: > PTAL. I'm planning to use these alternate ids for audio and video tracks to > extend test coverage for https://codereview.chromium.org/1812543003/ ping
On 2016/04/06 20:44:19, servolk wrote: > On 2016/04/05 23:00:35, servolk wrote: > > PTAL. I'm planning to use these alternate ids for audio and video tracks to > > extend test coverage for https://codereview.chromium.org/1812543003/ > > ping ping
On 2016/04/07 21:02:57, servolk wrote: > On 2016/04/06 20:44:19, servolk wrote: > > On 2016/04/05 23:00:35, servolk wrote: > > > PTAL. I'm planning to use these alternate ids for audio and video tracks to > > > extend test coverage for https://codereview.chromium.org/1812543003/ > > > > ping > > ping ping
wolenetz@chromium.org changed reviewers: - chcunningham@chromium.org
LGTM R-=chcunningham@ who is OoO. https://codereview.chromium.org/1860263002/diff/140001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1860263002/diff/140001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:313: DCHECK_EQ(audio_track_entry->data()[9], 0xD7); nit: we have 0xD7 defined and usable from here: https://code.google.com/p/chromium/codesearch#chromium/src/media/formats/webm... https://codereview.chromium.org/1860263002/diff/140001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:314: // Ensure the track id in TrackNum EBML element matches kAudioTrackNum. nit: since we're DCHECKing the TrackNum, should we also DCHECK that the size of the track id is 1 (actually, binary 1000 0001) in position [10]? https://codereview.chromium.org/1860263002/diff/140001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:328: DCHECK_EQ(video_track_entry->data()[9], 0xD7); nit ditto re-use defined constant https://codereview.chromium.org/1860263002/diff/140001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:329: // Ensure the track id in TrackNum EBML element matches kVideoTrackNum. nit ditto DCHECK track id size?
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/1860263002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1860263002/140001
The CQ bit was unchecked by wolenetz@chromium.org
https://codereview.chromium.org/1860263002/diff/140001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/1860263002/diff/140001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:313: DCHECK_EQ(audio_track_entry->data()[9], 0xD7); On 2016/04/08 22:41:49, wolenetz wrote: > nit: we have 0xD7 defined and usable from here: > https://code.google.com/p/chromium/codesearch#chromium/src/media/formats/webm... Done. https://codereview.chromium.org/1860263002/diff/140001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:314: // Ensure the track id in TrackNum EBML element matches kAudioTrackNum. On 2016/04/08 22:41:49, wolenetz wrote: > nit: since we're DCHECKing the TrackNum, should we also DCHECK that the size of > the track id is 1 (actually, binary 1000 0001) in position [10]? Done. https://codereview.chromium.org/1860263002/diff/140001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:328: DCHECK_EQ(video_track_entry->data()[9], 0xD7); On 2016/04/08 22:41:49, wolenetz wrote: > nit ditto re-use defined constant Done. https://codereview.chromium.org/1860263002/diff/140001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:329: // Ensure the track id in TrackNum EBML element matches kVideoTrackNum. On 2016/04/08 22:41:49, wolenetz wrote: > nit ditto DCHECK track id size? Done.
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/1860263002/#ps160001 (title: "CR feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1860263002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1860263002/160001
Message was sent while issue was closed.
Description was changed from ========== Support alternate audio/video track ids in ChunkDemuxer tests Also added a new unit test case where audio and video track ids change in the second init segment, and did some minor refactoring (renamed InitSegmentReceived -> InitSegmentReceivedMock). ========== to ========== Support alternate audio/video track ids in ChunkDemuxer tests Also added a new unit test case where audio and video track ids change in the second init segment, and did some minor refactoring (renamed InitSegmentReceived -> InitSegmentReceivedMock). ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Support alternate audio/video track ids in ChunkDemuxer tests Also added a new unit test case where audio and video track ids change in the second init segment, and did some minor refactoring (renamed InitSegmentReceived -> InitSegmentReceivedMock). ========== to ========== Support alternate audio/video track ids in ChunkDemuxer tests Also added a new unit test case where audio and video track ids change in the second init segment, and did some minor refactoring (renamed InitSegmentReceived -> InitSegmentReceivedMock). Committed: https://crrev.com/cb32bef64809077450a0e259053e3ef5734a543c Cr-Commit-Position: refs/heads/master@{#386248} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/cb32bef64809077450a0e259053e3ef5734a543c Cr-Commit-Position: refs/heads/master@{#386248} |
