Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
222783007:
Consider text tracks in the frame processor for new media segments. (Closed)
|
Created:
6 years, 8 months ago by DaleCurtis Modified:
6 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionConsider text tracks in the frame processor for new media segments.
Text tracks were not considered along with audio and video tracks
when determining the media segment start time. This results in
Append()s coming in later with timestamps before the segment start
time.
The issue issue is resolved and a DCHECK() added to SBS::Append() to
prevent this from happening in the future.
BUG=356805
TEST=media_unittests passes after DCHECK() added to Append().
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266636
Patch Set 1 #
Total comments: 8
Patch Set 2 : Comments. #
Total comments: 4
Patch Set 3 : Comments. #
Total comments: 2
Patch Set 4 : DCHECK. #
Messages
Total messages: 29 (0 generated)
Thanks for sharing. LegacyFrameProcessor must die.. soon :) First round of my comments: https://codereview.chromium.org/222783007/diff/1/media/filters/legacy_frame_p... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/222783007/diff/1/media/filters/legacy_frame_p... media/filters/legacy_frame_processor.cc:94: if (itr->second->stream()->type() != DemuxerStream::TEXT) |*new_media_segment| can become true between ProcessFrames() calls. For example, every time the stream parser identifies a new media segment is beginning, it sets this flag true. This change now excludes text tracks from recognizing new media segment starts or when audio or video discontinuities in the current media segment have been detected. The former may be ok (see http://crbug.com/351489), but perhaps we should retain some behavior like "if there's any resulting filtered text frames, and |*new_media_segment| became true prior to Append or during audio/video/text filtering, then also let text streams know OnNewMediaSegment"? The new FrameProcessor (WIP@ https://codereview.chromium.org/180153003/) complies more rigidly with spec especially around discontinuity detection. https://codereview.chromium.org/222783007/diff/1/media/filters/source_buffer_... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/222783007/diff/1/media/filters/source_buffer_... media/filters/source_buffer_stream_unittest.cc:367: stream_->OnNewMediaSegment(queue.front()->timestamp()); queue.front() must also be a keyframe, correct? Is a DCHECK for such appropriate here?
https://codereview.chromium.org/222783007/diff/1/media/filters/legacy_frame_p... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/222783007/diff/1/media/filters/legacy_frame_p... media/filters/legacy_frame_processor.cc:94: if (itr->second->stream()->type() != DemuxerStream::TEXT) On 2014/04/02 22:54:45, wolenetz wrote: > |*new_media_segment| can become true between ProcessFrames() calls. > For example, every time the stream parser identifies a new media segment is > beginning, it sets this flag true. > > This change now excludes text tracks from recognizing new media segment starts > or when audio or video discontinuities in the current media segment have been > detected. The former may be ok (see http://crbug.com/351489), but perhaps we > should retain some behavior like "if there's any resulting filtered text frames, > and |*new_media_segment| became true prior to Append or during audio/video/text > filtering, then also let text streams know OnNewMediaSegment"? The new > FrameProcessor (WIP@ https://codereview.chromium.org/180153003/) complies more > rigidly with spec especially around discontinuity detection. That sounds like overly complicated logic which wouldn't even catch most cases. I defer to you and acolwell@ on what you want to do here. https://codereview.chromium.org/222783007/diff/1/media/filters/source_buffer_... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/222783007/diff/1/media/filters/source_buffer_... media/filters/source_buffer_stream_unittest.cc:367: stream_->OnNewMediaSegment(queue.front()->timestamp()); On 2014/04/02 22:54:45, wolenetz wrote: > queue.front() must also be a keyframe, correct? Is a DCHECK for such appropriate > here? Well the test that is failing this is adding a non-keyframe first, so that will defeat the purpose of this fix.
https://codereview.chromium.org/222783007/diff/1/media/filters/legacy_frame_p... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/222783007/diff/1/media/filters/legacy_frame_p... media/filters/legacy_frame_processor.cc:94: if (itr->second->stream()->type() != DemuxerStream::TEXT) On 2014/04/02 23:02:27, DaleCurtis wrote: > On 2014/04/02 22:54:45, wolenetz wrote: > > |*new_media_segment| can become true between ProcessFrames() calls. > > For example, every time the stream parser identifies a new media segment is > > beginning, it sets this flag true. > > > > This change now excludes text tracks from recognizing new media segment starts > > or when audio or video discontinuities in the current media segment have been > > detected. The former may be ok (see http://crbug.com/351489), but perhaps we > > should retain some behavior like "if there's any resulting filtered text > frames, > > and |*new_media_segment| became true prior to Append or during > audio/video/text > > filtering, then also let text streams know OnNewMediaSegment"? The new > > FrameProcessor (WIP@ https://codereview.chromium.org/180153003/) complies more > > rigidly with spec especially around discontinuity detection. > > That sounds like overly complicated logic which wouldn't even catch most cases. > I defer to you and acolwell@ on what you want to do here. I believe the root of the problem is that the text track buffers are not being taken into account when computing segment_timestamp. Doing this would require interleaving the OnTextBuffers() code into this function. Video, audio, and text should all be using the same segment start timestamp. https://codereview.chromium.org/222783007/diff/1/media/filters/source_buffer_... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/222783007/diff/1/media/filters/source_buffer_... media/filters/source_buffer_stream.cc:450: DCHECK(media_segment_start_time_ <= buffers.front()->timestamp()); I was wrong. This should be using GetDecodeTimestamp() not timestamp(). I believe this will prevent the failures in the SourceBufferStream unit tests.
Comments addressed. PTAL. https://codereview.chromium.org/222783007/diff/1/media/filters/legacy_frame_p... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/222783007/diff/1/media/filters/legacy_frame_p... media/filters/legacy_frame_processor.cc:94: if (itr->second->stream()->type() != DemuxerStream::TEXT) On 2014/04/03 00:26:52, acolwell_OOO_4-6_to_4-10 wrote: > On 2014/04/02 23:02:27, DaleCurtis wrote: > > On 2014/04/02 22:54:45, wolenetz wrote: > > > |*new_media_segment| can become true between ProcessFrames() calls. > > > For example, every time the stream parser identifies a new media segment is > > > beginning, it sets this flag true. > > > > > > This change now excludes text tracks from recognizing new media segment > starts > > > or when audio or video discontinuities in the current media segment have > been > > > detected. The former may be ok (see http://crbug.com/351489), but perhaps we > > > should retain some behavior like "if there's any resulting filtered text > > frames, > > > and |*new_media_segment| became true prior to Append or during > > audio/video/text > > > filtering, then also let text streams know OnNewMediaSegment"? The new > > > FrameProcessor (WIP@ https://codereview.chromium.org/180153003/) complies > more > > > rigidly with spec especially around discontinuity detection. > > > > That sounds like overly complicated logic which wouldn't even catch most > cases. > > I defer to you and acolwell@ on what you want to do here. > > I believe the root of the problem is that the text track buffers are not being > taken into account when computing segment_timestamp. Doing this would require > interleaving the OnTextBuffers() code into this function. Video, audio, and text > should all be using the same segment start timestamp. Done. https://codereview.chromium.org/222783007/diff/1/media/filters/source_buffer_... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/222783007/diff/1/media/filters/source_buffer_... media/filters/source_buffer_stream.cc:450: DCHECK(media_segment_start_time_ <= buffers.front()->timestamp()); On 2014/04/03 00:26:52, acolwell_OOO_4-6_to_4-10 wrote: > I was wrong. This should be using GetDecodeTimestamp() not timestamp(). I > believe this will prevent the failures in the SourceBufferStream unit tests. Ah, good call, this fixed the issue.
lgtm % nits https://codereview.chromium.org/222783007/diff/20001/media/filters/legacy_fra... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/222783007/diff/20001/media/filters/legacy_fra... media/filters/legacy_frame_processor.cc:111: nit: Please add DCHECK(segment_timestamp != kInfiniteDuration()) here. https://codereview.chromium.org/222783007/diff/20001/media/filters/legacy_fra... media/filters/legacy_frame_processor.cc:259: track->set_needs_random_access_point(false); nit: I think you should be able to remove this call. All text track buffers should be marked as keyframes. If they aren't then we have a bug somewhere or the test data is wrong.
wolenetz: Any other comments? https://codereview.chromium.org/222783007/diff/20001/media/filters/legacy_fra... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/222783007/diff/20001/media/filters/legacy_fra... media/filters/legacy_frame_processor.cc:111: On 2014/04/11 18:02:00, acolwell wrote: > nit: Please add DCHECK(segment_timestamp != kInfiniteDuration()) here. Done. https://codereview.chromium.org/222783007/diff/20001/media/filters/legacy_fra... media/filters/legacy_frame_processor.cc:259: track->set_needs_random_access_point(false); On 2014/04/11 18:02:00, acolwell wrote: > nit: I think you should be able to remove this call. All text track buffers > should be marked as keyframes. If they aren't then we have a bug somewhere or > the test data is wrong. Done.
lgtm & nits: nit: Description references a two-part problem, one being an incorrect calculation in tests, though there is no test fix contained in the CL. Is description still accurate? https://codereview.chromium.org/222783007/diff/40001/media/filters/legacy_fra... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/222783007/diff/40001/media/filters/legacy_fra... media/filters/legacy_frame_processor.cc:271: filtered_text->insert(std::make_pair(text_track_id, filtered_buffers)); nit: Though I believe we don't currently attempt re-insertion of already-inserted key, we should protect against this here. Please add a DCHECK.
On 2014/04/14 21:41:15, wolenetz wrote: > lgtm & nits: > > nit: Description references a two-part problem, one being an incorrect > calculation in tests, though there is no test fix contained in the CL. Is > description still accurate? > > https://codereview.chromium.org/222783007/diff/40001/media/filters/legacy_fra... > File media/filters/legacy_frame_processor.cc (right): > > https://codereview.chromium.org/222783007/diff/40001/media/filters/legacy_fra... > media/filters/legacy_frame_processor.cc:271: > filtered_text->insert(std::make_pair(text_track_id, filtered_buffers)); > nit: Though I believe we don't currently attempt re-insertion of > already-inserted key, we should protect against this here. Please add a DCHECK. s/&/%
Added the DCHECK() and fixed the description, but the recent DefaultDuration change by sergeyu@ is causing new tests to fail :( [==========] Running 5 tests from 1 test case. [----------] Global test environment set-up. [----------] 5 tests from LegacyFrameProcessor/ChunkDemuxerTest [ RUN ] LegacyFrameProcessor/ChunkDemuxerTest.WebMFile_AudioAndVideo/0 [17783:17783:0414/145701:248980748225:FATAL:source_buffer_stream.cc(450)] Check failed: media_segment_start_time_ <= buffers.front()->timestamp(). 2002000 <= 1987000 [1/5] LegacyFrameProcessor/ChunkDemuxerTest.WebMFile_AudioAndVideo/0 (CRASHED) Note: Google Test filter = LegacyFrameProcessor/ChunkDemuxerTest.WebMFile_LiveAudioAndVideo/0 [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from LegacyFrameProcessor/ChunkDemuxerTest [ RUN ] LegacyFrameProcessor/ChunkDemuxerTest.WebMFile_LiveAudioAndVideo/0 [17784:17784:0414/145701:248980775759:FATAL:source_buffer_stream.cc(450)] Check failed: media_segment_start_time_ <= buffers.front()->timestamp(). 801000 <= 779000 [2/5] LegacyFrameProcessor/ChunkDemuxerTest.WebMFile_LiveAudioAndVideo/0 (CRASHED) wolenetz@ is taking a look. https://codereview.chromium.org/222783007/diff/40001/media/filters/legacy_fra... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/222783007/diff/40001/media/filters/legacy_fra... media/filters/legacy_frame_processor.cc:271: filtered_text->insert(std::make_pair(text_track_id, filtered_buffers)); On 2014/04/14 21:41:16, wolenetz wrote: > nit: Though I believe we don't currently attempt re-insertion of > already-inserted key, we should protect against this here. Please add a DCHECK. Done.
On 2014/04/14 22:11:51, DaleCurtis wrote: > Added the DCHECK() and fixed the description, but the recent DefaultDuration > change by sergeyu@ is causing new tests to fail :( > > [==========] Running 5 tests from 1 test case. > [----------] Global test environment set-up. > [----------] 5 tests from LegacyFrameProcessor/ChunkDemuxerTest > [ RUN ] LegacyFrameProcessor/ChunkDemuxerTest.WebMFile_AudioAndVideo/0 > [17783:17783:0414/145701:248980748225:FATAL:source_buffer_stream.cc(450)] Check > failed: media_segment_start_time_ <= buffers.front()->timestamp(). 2002000 <= > 1987000 > [1/5] LegacyFrameProcessor/ChunkDemuxerTest.WebMFile_AudioAndVideo/0 (CRASHED) > > Note: Google Test filter = > LegacyFrameProcessor/ChunkDemuxerTest.WebMFile_LiveAudioAndVideo/0 > [==========] Running 1 test from 1 test case. > [----------] Global test environment set-up. > [----------] 1 test from LegacyFrameProcessor/ChunkDemuxerTest > [ RUN ] LegacyFrameProcessor/ChunkDemuxerTest.WebMFile_LiveAudioAndVideo/0 > [17784:17784:0414/145701:248980775759:FATAL:source_buffer_stream.cc(450)] Check > failed: media_segment_start_time_ <= buffers.front()->timestamp(). 801000 <= > 779000 > [2/5] LegacyFrameProcessor/ChunkDemuxerTest.WebMFile_LiveAudioAndVideo/0 > (CRASHED) > > wolenetz@ is taking a look. > > https://codereview.chromium.org/222783007/diff/40001/media/filters/legacy_fra... > File media/filters/legacy_frame_processor.cc (right): > > https://codereview.chromium.org/222783007/diff/40001/media/filters/legacy_fra... > media/filters/legacy_frame_processor.cc:271: > filtered_text->insert(std::make_pair(text_track_id, filtered_buffers)); > On 2014/04/14 21:41:16, wolenetz wrote: > > nit: Though I believe we don't currently attempt re-insertion of > > already-inserted key, we should protect against this here. Please add a > DCHECK. > > Done. Regarding the error exposed (but not really caused by) sergeyu@'s change, see https://crbug.com/363421. I'm working on a fix, rough ETA == tomorrow.
On 2014/04/14 23:03:04, wolenetz wrote: > On 2014/04/14 22:11:51, DaleCurtis wrote: > > Added the DCHECK() and fixed the description, but the recent DefaultDuration > > change by sergeyu@ is causing new tests to fail :( > > > > [==========] Running 5 tests from 1 test case. > > [----------] Global test environment set-up. > > [----------] 5 tests from LegacyFrameProcessor/ChunkDemuxerTest > > [ RUN ] LegacyFrameProcessor/ChunkDemuxerTest.WebMFile_AudioAndVideo/0 > > [17783:17783:0414/145701:248980748225:FATAL:source_buffer_stream.cc(450)] > Check > > failed: media_segment_start_time_ <= buffers.front()->timestamp(). 2002000 <= > > 1987000 > > [1/5] LegacyFrameProcessor/ChunkDemuxerTest.WebMFile_AudioAndVideo/0 (CRASHED) > > > > Note: Google Test filter = > > LegacyFrameProcessor/ChunkDemuxerTest.WebMFile_LiveAudioAndVideo/0 > > [==========] Running 1 test from 1 test case. > > [----------] Global test environment set-up. > > [----------] 1 test from LegacyFrameProcessor/ChunkDemuxerTest > > [ RUN ] > LegacyFrameProcessor/ChunkDemuxerTest.WebMFile_LiveAudioAndVideo/0 > > [17784:17784:0414/145701:248980775759:FATAL:source_buffer_stream.cc(450)] > Check > > failed: media_segment_start_time_ <= buffers.front()->timestamp(). 801000 <= > > 779000 > > [2/5] LegacyFrameProcessor/ChunkDemuxerTest.WebMFile_LiveAudioAndVideo/0 > > (CRASHED) > > > > wolenetz@ is taking a look. > > > > > https://codereview.chromium.org/222783007/diff/40001/media/filters/legacy_fra... > > File media/filters/legacy_frame_processor.cc (right): > > > > > https://codereview.chromium.org/222783007/diff/40001/media/filters/legacy_fra... > > media/filters/legacy_frame_processor.cc:271: > > filtered_text->insert(std::make_pair(text_track_id, filtered_buffers)); > > On 2014/04/14 21:41:16, wolenetz wrote: > > > nit: Though I believe we don't currently attempt re-insertion of > > > already-inserted key, we should protect against this here. Please add a > > DCHECK. > > > > Done. > > Regarding the error exposed (but not really caused by) sergeyu@'s change, see > https://crbug.com/363421. I'm working on a fix, rough ETA == tomorrow. My work-in-progress CL is at https://codereview.chromium.org/239343007/.
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/222783007/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/222783007/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by dalecurtis@chromium.org
The CQ bit was unchecked by dalecurtis@chromium.org
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/222783007/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/222783007/60001
Message was sent while issue was closed.
Change committed as 266636 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
