Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
276573002:
Add gapless playback support for AAC playback. (Closed)
|
Created:
6 years, 7 months ago by DaleCurtis Modified:
6 years, 6 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd gapless playback support for AAC playback.
Gapless support for AAC is similar to MP3 but unlike
MP3 which only has padding in the first frame, AAC may
require discarding data across multiple frames.
Unfortunately this means we can't just discard the padding
frames wholesale without changing what's decoded from the
first non-padding frame.
Testing reveals we need to decode the padding frame prior to
the first non-padding frame in order to correctly decode the
non-padding frame.
We accomplish this by introducing the concept of a preroll
buffer which is to be played out prior to the current buffer
but must be entirely discarded. Luckily this is easily done
with the infrastructure in place for the gapless MP3 playback!
While we need to support post splice buffers with preroll for
gapless playback, for simplicity, pre-splice buffers are not
allowed to have preroll.
In sum, this change:
- Adds SetPrerollBuffer() and preroll_buffer() to the
StreamParserBuffer.
- Refactors splice frame handling into a new
HandleNextBufferWithSpliceFrame() method, introduces
HandleNextBufferWithPreroll()
- Modifies the frame processor to use the frame prior to the
partial append window discard as the preroll.
BUG=371633
TEST=new unittests!
R=wolenetz@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274073
Patch Set 1 : No config change! #Patch Set 2 : Now with tests! #
Total comments: 37
Patch Set 3 : Rebase. Comments. #
Total comments: 49
Patch Set 4 : Comments. #Patch Set 5 : Rebase. Add config change test. #
Total comments: 2
Patch Set 6 : Rebase. Comments. #
Total comments: 20
Patch Set 7 : Comments. #Patch Set 8 : Fix per offline comments. #
Total comments: 2
Patch Set 9 : Comments. #Patch Set 10 : Fix msvc error. #
Messages
Total messages: 28 (0 generated)
Please take a look when you have time, but don't worry about looking at this until after the branch cut. Thanks! Partial appends and splice frames for AAC, plus a new test are enabled in a separate CL which I'll land after this one: https://codereview.chromium.org/278673003/ This CL will require updating once wolentz@ lands his new frame processor too.
https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... File media/base/stream_parser_buffer.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... media/base/stream_parser_buffer.cc:184: preroll_buffer_->set_timestamp(timestamp()); This seems hacky to do all this setting on a get operation. Why can't this all be done on the set? https://codereview.chromium.org/276573002/diff/60001/media/filters/legacy_fra... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/legacy_fra... media/filters/legacy_frame_processor.cc:219: if (itr != buffers.begin()) This looks like it would only work if both buffers are in the same append. I don't think this behavior should have to depend on that. https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... media/filters/source_buffer_stream.cc:1158: if (!pending_buffer_) { It seems weird to have this here. Why not put this code in GetNextBuffer() where you detect a new splice buffer? That would at least make it more obvious that pending_buffer_ gets set. https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... media/filters/source_buffer_stream.cc:1222: pending_buffer_.swap(*out_buffer); This seems odd as well. I think the setting operation should be split out so that this is more explicit at the call sites.
https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... File media/base/stream_parser_buffer.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... media/base/stream_parser_buffer.cc:184: preroll_buffer_->set_timestamp(timestamp()); On 2014/05/16 17:01:11, acolwell wrote: > This seems hacky to do all this setting on a get operation. Why can't this all > be done on the set? If something comes along later and calls any Set() methods on the buffer the preroll buffer needs to be updated. I found this preferable to changing every setter to have an "if (preroll) preroll->set_xxx()" clause. https://codereview.chromium.org/276573002/diff/60001/media/filters/legacy_fra... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/legacy_fra... media/filters/legacy_frame_processor.cc:219: if (itr != buffers.begin()) On 2014/05/16 17:01:11, acolwell wrote: > This looks like it would only work if both buffers are in the same append. I > don't think this behavior should have to depend on that. Hmm, since buffers outside the append window are lost forever, seemingly the only other way would be to always keep the last buffer discarded before the appendWindowStart across calls. Is that something you're comfortable with? https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... media/filters/source_buffer_stream.cc:1158: if (!pending_buffer_) { On 2014/05/16 17:01:11, acolwell wrote: > It seems weird to have this here. Why not put this code in GetNextBuffer() where > you detect a new splice buffer? That would at least make it more obvious that > pending_buffer_ gets set. This is done for consistency with the HandleNextBufferWithPreroll() method below. It's done there since HandleNextBufferWithSplice() can flow into HandleNextBufferWithPreroll() afterward. I can extract a "set pending buffer" method, but it might lose some of it's DCHECKs if I move it outside of each Handle method. https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... media/filters/source_buffer_stream.cc:1222: pending_buffer_.swap(*out_buffer); On 2014/05/16 17:01:11, acolwell wrote: > This seems odd as well. I think the setting operation should be split out so > that this is more explicit at the call sites. As detailed above.
https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... File media/base/stream_parser_buffer.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... media/base/stream_parser_buffer.cc:184: preroll_buffer_->set_timestamp(timestamp()); On 2014/05/16 17:48:59, DaleCurtis wrote: > On 2014/05/16 17:01:11, acolwell wrote: > > This seems hacky to do all this setting on a get operation. Why can't this all > > be done on the set? > > If something comes along later and calls any Set() methods on the buffer the > preroll buffer needs to be updated. I found this preferable to changing every > setter to have an "if (preroll) preroll->set_xxx()" clause. But at least the that is a more direct reflection of what is going on. Please update the setters instead of doing this. https://codereview.chromium.org/276573002/diff/60001/media/filters/legacy_fra... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/legacy_fra... media/filters/legacy_frame_processor.cc:219: if (itr != buffers.begin()) On 2014/05/16 17:48:59, DaleCurtis wrote: > On 2014/05/16 17:01:11, acolwell wrote: > > This looks like it would only work if both buffers are in the same append. I > > don't think this behavior should have to depend on that. > > Hmm, since buffers outside the append window are lost forever, seemingly the > only other way would be to always keep the last buffer discarded before the > appendWindowStart across calls. Is that something you're comfortable with? If that is what is necessary. Appending 1 byte at a time should result in the same outcome as appending the whole thing at once. Obviously having a test to verify this would be nice. :) https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... media/filters/source_buffer_stream.cc:1158: if (!pending_buffer_) { On 2014/05/16 17:48:59, DaleCurtis wrote: > On 2014/05/16 17:01:11, acolwell wrote: > > It seems weird to have this here. Why not put this code in GetNextBuffer() > where > > you detect a new splice buffer? That would at least make it more obvious that > > pending_buffer_ gets set. > > This is done for consistency with the HandleNextBufferWithPreroll() method > below. It's done there since HandleNextBufferWithSplice() can flow into > HandleNextBufferWithPreroll() afterward. I can extract a "set pending buffer" > method, but it might lose some of it's DCHECKs if I move it outside of each > Handle method. I think the setting logic should be extracted into a helper and the helper called at the relevant call sites instead of in this and HandleNextBufferWithSplice(). I realize that might make the code at the bottom of this func a little longer, but I think it will be a little clearer that pending_buffers_ gets reset in the buffers with preroll case.
https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... File media/base/stream_parser_buffer.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... media/base/stream_parser_buffer.cc:184: preroll_buffer_->set_timestamp(timestamp()); On 2014/05/16 20:26:05, acolwell wrote: > On 2014/05/16 17:48:59, DaleCurtis wrote: > > On 2014/05/16 17:01:11, acolwell wrote: > > > This seems hacky to do all this setting on a get operation. Why can't this > all > > > be done on the set? > > > > If something comes along later and calls any Set() methods on the buffer the > > preroll buffer needs to be updated. I found this preferable to changing every > > setter to have an "if (preroll) preroll->set_xxx()" clause. > > But at least the that is a more direct reflection of what is going on. Please > update the setters instead of doing this. It's not that simple since set_timestamp() is a hacker_style() method on DecoderBuffer which is not-virtual. My preference is not to gum up the setters with this complexity.
https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... File media/base/stream_parser_buffer.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... media/base/stream_parser_buffer.cc:167: DCHECK(preroll_buffer->timestamp() < timestamp()); nit: Is this strictly always true, even for out-of-order decode stream types that might use preroll, or should we s/</<=/ ? https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... media/base/stream_parser_buffer.cc:169: DCHECK_EQ(preroll_buffer->type(), type()); Could the app cause decoder problems by causing a situation where the preroll buffer really is meant to be decoded with a different config than *this? For example, append(buffer_with_config1+buffer_with_config2) with append_window clipping set to use decoded frames from somewhere in the middle of buffer_with_config2? This might be a special case similar to acolwell's comments, below, too. https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... media/base/stream_parser_buffer.cc:173: // assumption when working with WebM... suggestion (possibly for a different CL): could discard_padding use kInfiniteDuration() to indicate this "discard all resulting decoded frames" condition? https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... media/base/stream_parser_buffer.cc:181: if (preroll_buffer_) { nit: reverse condition and return early. https://codereview.chromium.org/276573002/diff/60001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:3079: // Set the append window to [50,280). Why increase 20 -> 50? Currently, video does not support partial append window trimming, so preroll (or even partial append window trimming itself) shouldn't change the result, below, right? Perhaps video keyframe preroll and partial append window trimming should be supported for video? (separate CL) https://codereview.chromium.org/276573002/diff/60001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:3093: // Extend the append window to [20,650). nit: Fix comment if we keep to 50ms window start. https://codereview.chromium.org/276573002/diff/60001/media/filters/legacy_fra... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/legacy_fra... media/filters/legacy_frame_processor.cc:199: frame_end_timestamp > append_window_start && clarification: For all ADTS streams (or any that may eventually require similar preroll), the preroll should end partially through a buffer, and never exactly at a frame_end_timestamp boundary, right? (If it could occur at a boundary, then this code needs to be fixed to support prerolling even in that case.) https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... media/filters/source_buffer_stream.cc:1227: DCHECK(pending_buffer_->get_splice_buffers().empty()); nit: is it worth adding DCHECK_EQ(0, splice_buffers_index_); here too? https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:313: ASSERT_TRUE(buffer->IsKeyframe()); Must preroll be followed by a keyframe? Consider if we allow keyframe video to be preroll; then next frame doesn't need to be one, or am I missing something?
On 2014/05/17 00:41:18, wolenetz wrote: > https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... > File media/base/stream_parser_buffer.cc (right): > > https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... > media/base/stream_parser_buffer.cc:167: DCHECK(preroll_buffer->timestamp() < > timestamp()); > nit: Is this strictly always true, even for out-of-order decode stream types > that might use preroll, or should we s/</<=/ ? > > https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... > media/base/stream_parser_buffer.cc:169: DCHECK_EQ(preroll_buffer->type(), > type()); > Could the app cause decoder problems by causing a situation where the preroll > buffer really is meant to be decoded with a different config than *this? For > example, append(buffer_with_config1+buffer_with_config2) with append_window > clipping set to use decoded frames from somewhere in the middle of > buffer_with_config2? > This might be a special case similar to acolwell's comments, below, too. > > https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... > media/base/stream_parser_buffer.cc:173: // assumption when working with WebM... > suggestion (possibly for a different CL): could discard_padding use > kInfiniteDuration() to indicate this "discard all resulting decoded frames" > condition? > > https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... > media/base/stream_parser_buffer.cc:181: if (preroll_buffer_) { > nit: reverse condition and return early. > > https://codereview.chromium.org/276573002/diff/60001/media/filters/chunk_demu... > File media/filters/chunk_demuxer_unittest.cc (right): > > https://codereview.chromium.org/276573002/diff/60001/media/filters/chunk_demu... > media/filters/chunk_demuxer_unittest.cc:3079: // Set the append window to > [50,280). > Why increase 20 -> 50? Currently, video does not support partial append window > trimming, so preroll (or even partial append window trimming itself) shouldn't > change the result, below, right? > Perhaps video keyframe preroll and partial append window trimming should be > supported for video? (separate CL) > > https://codereview.chromium.org/276573002/diff/60001/media/filters/chunk_demu... > media/filters/chunk_demuxer_unittest.cc:3093: // Extend the append window to > [20,650). > nit: Fix comment if we keep to 50ms window start. > > https://codereview.chromium.org/276573002/diff/60001/media/filters/legacy_fra... > File media/filters/legacy_frame_processor.cc (right): > > https://codereview.chromium.org/276573002/diff/60001/media/filters/legacy_fra... > media/filters/legacy_frame_processor.cc:199: frame_end_timestamp > > append_window_start && > clarification: For all ADTS streams (or any that may eventually require similar > preroll), the preroll should end partially through a buffer, and never exactly > at a frame_end_timestamp boundary, right? (If it could occur at a boundary, then > this code needs to be fixed to support prerolling even in that case.) > > https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... > File media/filters/source_buffer_stream.cc (right): > > https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... > media/filters/source_buffer_stream.cc:1227: > DCHECK(pending_buffer_->get_splice_buffers().empty()); > nit: is it worth adding DCHECK_EQ(0, splice_buffers_index_); here too? > > https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... > File media/filters/source_buffer_stream_unittest.cc (right): > > https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... > media/filters/source_buffer_stream_unittest.cc:313: > ASSERT_TRUE(buffer->IsKeyframe()); > Must preroll be followed by a keyframe? Consider if we allow keyframe video to > be preroll; then next frame doesn't need to be one, or am I missing something? Also, FrameProcessor has landed, so it will need similar changes made to it in this CL.
https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... File media/base/stream_parser_buffer.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... media/base/stream_parser_buffer.cc:167: DCHECK(preroll_buffer->timestamp() < timestamp()); On 2014/05/17 00:41:17, wolenetz wrote: > nit: Is this strictly always true, even for out-of-order decode stream types > that might use preroll, or should we s/</<=/ ? Hmm, it might need to be <= since that's what the IsRangeListSorted() method uses. I didn't think it's possible to have buffers with the same timestamp within a single append. https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... media/base/stream_parser_buffer.cc:169: DCHECK_EQ(preroll_buffer->type(), type()); On 2014/05/17 00:41:17, wolenetz wrote: > Could the app cause decoder problems by causing a situation where the preroll > buffer really is meant to be decoded with a different config than *this? For > example, append(buffer_with_config1+buffer_with_config2) with append_window > clipping set to use decoded frames from somewhere in the middle of > buffer_with_config2? > This might be a special case similar to acolwell's comments, below, too. Yes this would be a problem. They must have the same config though or preroll doesn't make sense since the decode would be flushed in between them. I haven't yet looked at Aaron's request to handle preroll across appends, but to do so we would definitely need to invalidate the buffer kept across appends if append_config_index changes. https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... media/base/stream_parser_buffer.cc:173: // assumption when working with WebM... On 2014/05/17 00:41:17, wolenetz wrote: > suggestion (possibly for a different CL): could discard_padding use > kInfiniteDuration() to indicate this "discard all resulting decoded frames" > condition? That's a good idea. I'll work it into the next patch set. https://codereview.chromium.org/276573002/diff/60001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:3079: // Set the append window to [50,280). On 2014/05/17 00:41:17, wolenetz wrote: > Why increase 20 -> 50? Currently, video does not support partial append window > trimming, so preroll (or even partial append window trimming itself) shouldn't > change the result, below, right? > Perhaps video keyframe preroll and partial append window trimming should be > supported for video? (separate CL) I changed them both because I thought it was a better test. https://codereview.chromium.org/276573002/diff/60001/media/filters/legacy_fra... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/legacy_fra... media/filters/legacy_frame_processor.cc:199: frame_end_timestamp > append_window_start && On 2014/05/17 00:41:17, wolenetz wrote: > clarification: For all ADTS streams (or any that may eventually require similar > preroll), the preroll should end partially through a buffer, and never exactly > at a frame_end_timestamp boundary, right? (If it could occur at a boundary, then > this code needs to be fixed to support prerolling even in that case.) Yeah, that's correct, if it happened on a boundary this code would be wrong. I'll rework it with Aaron's request about supporting preroll across appends. https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... media/filters/source_buffer_stream_unittest.cc:313: ASSERT_TRUE(buffer->IsKeyframe()); On 2014/05/17 00:41:18, wolenetz wrote: > Must preroll be followed by a keyframe? Consider if we allow keyframe video to > be preroll; then next frame doesn't need to be one, or am I missing something? It's audio only at the moment, so yes it must be a keyframe. If we end up supporting video we can change it.
PTAL. All comments addressed. https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... File media/base/stream_parser_buffer.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... media/base/stream_parser_buffer.cc:167: DCHECK(preroll_buffer->timestamp() < timestamp()); On 2014/05/17 00:55:41, DaleCurtis wrote: > On 2014/05/17 00:41:17, wolenetz wrote: > > nit: Is this strictly always true, even for out-of-order decode stream types > > that might use preroll, or should we s/</<=/ ? > > Hmm, it might need to be <= since that's what the IsRangeListSorted() method > uses. I didn't think it's possible to have buffers with the same timestamp > within a single append. Done. https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... media/base/stream_parser_buffer.cc:173: // assumption when working with WebM... On 2014/05/17 00:55:41, DaleCurtis wrote: > On 2014/05/17 00:41:17, wolenetz wrote: > > suggestion (possibly for a different CL): could discard_padding use > > kInfiniteDuration() to indicate this "discard all resulting decoded frames" > > condition? > > That's a good idea. I'll work it into the next patch set. Done here: https://codereview.chromium.org/293053005/ https://codereview.chromium.org/276573002/diff/60001/media/base/stream_parser... media/base/stream_parser_buffer.cc:181: if (preroll_buffer_) { On 2014/05/17 00:41:17, wolenetz wrote: > nit: reverse condition and return early. Removed completely now per acolwell's request to move all this into the setters. https://codereview.chromium.org/276573002/diff/60001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:3093: // Extend the append window to [20,650). On 2014/05/17 00:41:17, wolenetz wrote: > nit: Fix comment if we keep to 50ms window start. Done. https://codereview.chromium.org/276573002/diff/60001/media/filters/legacy_fra... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/legacy_fra... media/filters/legacy_frame_processor.cc:199: frame_end_timestamp > append_window_start && On 2014/05/17 00:55:41, DaleCurtis wrote: > On 2014/05/17 00:41:17, wolenetz wrote: > > clarification: For all ADTS streams (or any that may eventually require > similar > > preroll), the preroll should end partially through a buffer, and never exactly > > at a frame_end_timestamp boundary, right? (If it could occur at a boundary, > then > > this code needs to be fixed to support prerolling even in that case.) > > Yeah, that's correct, if it happened on a boundary this code would be wrong. > I'll rework it with Aaron's request about supporting preroll across appends. Done. https://codereview.chromium.org/276573002/diff/60001/media/filters/legacy_fra... media/filters/legacy_frame_processor.cc:219: if (itr != buffers.begin()) On 2014/05/16 20:26:05, acolwell wrote: > On 2014/05/16 17:48:59, DaleCurtis wrote: > > On 2014/05/16 17:01:11, acolwell wrote: > > > This looks like it would only work if both buffers are in the same append. I > > > don't think this behavior should have to depend on that. > > > > Hmm, since buffers outside the append window are lost forever, seemingly the > > only other way would be to always keep the last buffer discarded before the > > appendWindowStart across calls. Is that something you're comfortable with? > > If that is what is necessary. Appending 1 byte at a time should result in the > same outcome as appending the whole thing at once. Obviously having a test to > verify this would be nice. :) Done. https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... media/filters/source_buffer_stream.cc:1158: if (!pending_buffer_) { On 2014/05/16 20:26:05, acolwell wrote: > On 2014/05/16 17:48:59, DaleCurtis wrote: > > On 2014/05/16 17:01:11, acolwell wrote: > > > It seems weird to have this here. Why not put this code in GetNextBuffer() > > where > > > you detect a new splice buffer? That would at least make it more obvious > that > > > pending_buffer_ gets set. > > > > This is done for consistency with the HandleNextBufferWithPreroll() method > > below. It's done there since HandleNextBufferWithSplice() can flow into > > HandleNextBufferWithPreroll() afterward. I can extract a "set pending buffer" > > method, but it might lose some of it's DCHECKs if I move it outside of each > > Handle method. > > I think the setting logic should be extracted into a helper and the helper > called at the relevant call sites instead of in this and > HandleNextBufferWithSplice(). I realize that might make the code at the bottom > of this func a little longer, but I think it will be a little clearer that > pending_buffers_ gets reset in the buffers with preroll case. Done. https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... media/filters/source_buffer_stream.cc:1222: pending_buffer_.swap(*out_buffer); On 2014/05/16 17:48:59, DaleCurtis wrote: > On 2014/05/16 17:01:11, acolwell wrote: > > This seems odd as well. I think the setting operation should be split out so > > that this is more explicit at the call sites. > > As detailed above. Done. https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... media/filters/source_buffer_stream.cc:1227: DCHECK(pending_buffer_->get_splice_buffers().empty()); On 2014/05/17 00:41:17, wolenetz wrote: > nit: is it worth adding DCHECK_EQ(0, splice_buffers_index_); here too? Moved into SetPendingBuffer().
looks pretty good. Just a few comments https://codereview.chromium.org/276573002/diff/80001/media/base/stream_parser... File media/base/stream_parser_buffer.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/base/stream_parser... media/base/stream_parser_buffer.cc:176: preroll_buffer_->track_id_ = track_id_; Why do you have to do this? I would expect these buffers to have the same track_id. If they don't is seems like there would be a problem upstream since it doesn't make sense to have a buffer from one track be the preroll for a different one. https://codereview.chromium.org/276573002/diff/80001/media/base/stream_parser... media/base/stream_parser_buffer.cc:179: preroll_buffer_->SetConfigId(GetConfigId()); Why do you need this? It seems like there would be a problem upstream if these got out of sync. I don't think this object really has enough information to be sure that this is a safe thing to do. ISTM that you should be asserting that the configID's match instead. https://codereview.chromium.org/276573002/diff/80001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:896: ASSERT_EQ(buffer->timestamp(), preroll_buffer->timestamp()); nit: remove? The expectations string should be able to capture this. Removing this also means you don't have too do the special extra Read() above. https://codereview.chromium.org/276573002/diff/80001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:3137: // Set the append window to [50,100). nit: s/100/150/ since the comment doesn't match the code? https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor.cc:276: frame_duration = frame->duration(); Since presentation_timestamp & frame_duration are being updated here, does frame_end_timestamp need to be recomputed? https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor.cc:277: } else if (presentation_timestamp < append_window_start || I wonder if we should drop the else here. ISTM that if the partial code above does its job right, then it should not trigger the code in this block to run. That would at least make it easier to see that the partial code can't accidentally violate what the spec requires. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... File media/filters/frame_processor_base.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.cc:106: // Ignore any buffers which start after |append_window_start| or end after DCHECK(DemuxerStream::AUDIO, buffer->type()) since all this code is assuming audio right now. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.cc:148: buffer->set_timestamp(append_window_start); Shouldn't these 2 calls also only need to happen if the condition below is true? If this is just to cause the preroll buffer timestamps to get changed, should that occur in the block above instead so it is more explicit? https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... File media/filters/frame_processor_base.h (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.h:181: // the extents of |buffer|. This seems a little odd. Is there any reason we can't update |buffer|'s timestamps before this function is called so we don't have to have these parameters? This feels like it is leaking details of the caller's implementation here. https://codereview.chromium.org/276573002/diff/80001/media/filters/source_buf... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/source_buf... media/filters/source_buffer_stream.cc:2242: DCHECK(!have_splice_buffers); I think something like DCHECK_NE(have_splice_buffers, have_preroll_buffer); is closer to what you are actually trying to enforce here.
looking pretty good: https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/276573002/diff/60001/media/filters/source_buf... media/filters/source_buffer_stream.cc:1227: DCHECK(pending_buffer_->get_splice_buffers().empty()); On 2014/05/22 23:58:57, DaleCurtis wrote: > On 2014/05/17 00:41:17, wolenetz wrote: > > nit: is it worth adding DCHECK_EQ(0, splice_buffers_index_); here too? > > Moved into SetPendingBuffer(). The set is moved into SetPendingBuffer(), though the DCHECK_EQ I suggested would still be good here to protect borders. https://codereview.chromium.org/276573002/diff/80001/media/base/stream_parser... File media/base/stream_parser_buffer.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/base/stream_parser... media/base/stream_parser_buffer.cc:171: DCHECK(preroll_buffer->timestamp() <= timestamp()); Please add DCHECK(preroll_buffer->GetDecodeTimestamp() <= GetDecodeTimestamp()); I'm concerned that so much of SourceBufferStream assumes non-decreasing PTS in many cases, which is fine for webm, but not fine for out-of-order stream types. The assumption should at least include non-decreasing DTS, and in some cases allow PTS to decrease. I'm uncertain the timestamp() check is good: are there any out-of-order decodes allowed for any stream types that use SetPrerollBuffer? https://codereview.chromium.org/276573002/diff/80001/media/filters/chunk_demu... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/chunk_demu... media/filters/chunk_demuxer.cc:571: frame_processor_->clear_audio_preroll_buffer(); I would like to see a test that preroll is cleared if config change occurs prior to next buffer even if next buffer abuts the preroll buffer. https://codereview.chromium.org/276573002/diff/80001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:3099: // The first 50ms of the first buffer should be trimmed off since it nit: include mention in comment that the 'first buffer' includes preroll of the '0K', plus the '30K'? https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor.cc:273: // |frame| has been partially trimmed or had preroll added. ditto of LegacyFrameProcessor comment: do the following here, too? track_buffer->set_needs_random_access_point(true); if (!sequence_mode_) *new_media_segment = true; https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor.cc:277: } else if (presentation_timestamp < append_window_start || On 2014/05/23 17:27:30, acolwell wrote: > I wonder if we should drop the else here. ISTM that if the partial code above > does its job right, then it should not trigger the code in this block to run. > That would at least make it easier to see that the partial code can't > accidentally violate what the spec requires. Good point: I think that would address my *new_media_segment/set_needs_random_access_point comments, too. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... File media/filters/frame_processor_base.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.cc:122: audio_preroll_buffer_->set_timestamp(frame_start_timestamp); also SetDecodeTimestamp()? https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.cc:143: } nit: MEDIA_LOG when an audio_preroll_buffer exists but does *not* abut the next buffer? https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... File media/filters/frame_processor_base.h (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.h:186: // preroll on |buffer| and |audio_preroll_buffer_| will be cleared. We should also clear |audio_preroll_buffer_| if it exists but does not abut the next |buffer|, right? https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.h:190: // timestamp and duration fields will also be set accordingly. Please clarify that false is returned in this case. This also seems a little odd/leaking caller's implementation in that the assumption here is that |buffer| is then expected to be dropped by caller and a subsequent |buffer| is expected to be given |audio_preroll_buffer_| if it is immediately next. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.h:218: scoped_refptr<StreamParserBuffer> audio_preroll_buffer_; nit: Please also clarify the unstated assumption that at most one |audio_preroll_buffer_| exists across all tracks managed by the frame processor. If we ever allow multiplexed audio tracks in a stream that supports partial append window trimming, the assumption would break. https://codereview.chromium.org/276573002/diff/80001/media/filters/legacy_fra... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/legacy_fra... media/filters/legacy_frame_processor.cc:193: // |buffer| has been partially trimmed or had preroll added. In this case, does this mean that a discontinuity has occurred such that the track's SourceBufferStream will need to be told that a new media segment has started, and not always will a preroll buffer have been added to |buffer|? (I think so, so please add *new_media_segment = true; here) ditto for track->set_needs_random_access_point(true);
https://codereview.chromium.org/276573002/diff/80001/media/base/stream_parser... File media/base/stream_parser_buffer.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/base/stream_parser... media/base/stream_parser_buffer.cc:171: DCHECK(preroll_buffer->timestamp() <= timestamp()); On 2014/05/23 19:59:45, wolenetz wrote: > Please add DCHECK(preroll_buffer->GetDecodeTimestamp() <= GetDecodeTimestamp()); > I'm concerned that so much of SourceBufferStream assumes non-decreasing PTS in > many cases, which is fine for webm, but not fine for out-of-order stream types. > The assumption should at least include non-decreasing DTS, and in some cases > allow PTS to decrease. > I'm uncertain the timestamp() check is good: are there any out-of-order decodes > allowed for any stream types that use SetPrerollBuffer? Currently I ignore the decode timestamp on the preroll buffer, so that check wouldn't do anything. The preroll buffer gets whatever decode timestamp the containing buffer has. https://codereview.chromium.org/276573002/diff/80001/media/base/stream_parser... media/base/stream_parser_buffer.cc:176: preroll_buffer_->track_id_ = track_id_; On 2014/05/23 17:27:30, acolwell wrote: > Why do you have to do this? I would expect these buffers to have the same > track_id. If they don't is seems like there would be a problem upstream since it > doesn't make sense to have a buffer from one track be the preroll for a > different one. I was just wholesale copying here, looking at it more closely, you're correct and that we shouldn't have to do this. Changed to a DCHECK(). https://codereview.chromium.org/276573002/diff/80001/media/base/stream_parser... media/base/stream_parser_buffer.cc:179: preroll_buffer_->SetConfigId(GetConfigId()); On 2014/05/23 17:27:30, acolwell wrote: > Why do you need this? It seems like there would be a problem upstream if these > got out of sync. I don't think this object really has enough information to be > sure that this is a safe thing to do. ISTM that you should be asserting that the > configID's match instead. Because a config ID hasn't yet been assigned to this buffer. That's not done until SourceBufferStream::Append() by SBS::SetConfigIDs(). https://codereview.chromium.org/276573002/diff/80001/media/filters/chunk_demu... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/chunk_demu... media/filters/chunk_demuxer.cc:571: frame_processor_->clear_audio_preroll_buffer(); On 2014/05/23 19:59:45, wolenetz wrote: > I would like to see a test that preroll is cleared if config change occurs prior > to next buffer even if next buffer abuts the preroll buffer. I'll address this in the next patch set. It's proving really tricky to write a test to do this and I want to get this up for review before acolwell@ heads out on vacation. https://codereview.chromium.org/276573002/diff/80001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:896: ASSERT_EQ(buffer->timestamp(), preroll_buffer->timestamp()); On 2014/05/23 17:27:30, acolwell wrote: > nit: remove? The expectations string should be able to capture this. Removing > this also means you don't have too do the special extra Read() above. Done. https://codereview.chromium.org/276573002/diff/80001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:3099: // The first 50ms of the first buffer should be trimmed off since it On 2014/05/23 19:59:45, wolenetz wrote: > nit: include mention in comment that the 'first buffer' includes preroll of the > '0K', plus the '30K'? Done. https://codereview.chromium.org/276573002/diff/80001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:3137: // Set the append window to [50,100). On 2014/05/23 17:27:30, acolwell wrote: > nit: s/100/150/ since the comment doesn't match the code? Done. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor.cc:273: // |frame| has been partially trimmed or had preroll added. On 2014/05/23 19:59:45, wolenetz wrote: > ditto of LegacyFrameProcessor comment: do the following here, too? > track_buffer->set_needs_random_access_point(true); > if (!sequence_mode_) > *new_media_segment = true; Blew out the else from below. Should be fine now? https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor.cc:276: frame_duration = frame->duration(); On 2014/05/23 17:27:30, acolwell wrote: > Since presentation_timestamp & frame_duration are being updated here, does > frame_end_timestamp need to be recomputed? No, since the timestamp is moved forward by a value equal to that by which the duration is shrunk. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor.cc:277: } else if (presentation_timestamp < append_window_start || On 2014/05/23 17:27:30, acolwell wrote: > I wonder if we should drop the else here. ISTM that if the partial code above > does its job right, then it should not trigger the code in this block to run. > That would at least make it easier to see that the partial code can't > accidentally violate what the spec requires. Seems reasonable. Done. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... File media/filters/frame_processor_base.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.cc:106: // Ignore any buffers which start after |append_window_start| or end after On 2014/05/23 17:27:30, acolwell wrote: > DCHECK(DemuxerStream::AUDIO, buffer->type()) since all this code is assuming > audio right now. Done. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.cc:122: audio_preroll_buffer_->set_timestamp(frame_start_timestamp); On 2014/05/23 19:59:45, wolenetz wrote: > also SetDecodeTimestamp()? I don't think it's necessary. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.cc:143: } On 2014/05/23 19:59:45, wolenetz wrote: > nit: MEDIA_LOG when an audio_preroll_buffer exists but does *not* abut the next > buffer? For another CL, MEDIA_LOG isn't available here. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.cc:148: buffer->set_timestamp(append_window_start); On 2014/05/23 17:27:30, acolwell wrote: > Shouldn't these 2 calls also only need to happen if the condition below is true? > If this is just to cause the preroll buffer timestamps to get changed, should > that occur in the block above instead so it is more explicit? Correct. Seems unnecessary to duplicate these sets, but if you feel strongly I can do so. I've added a clarifying comment at least. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... File media/filters/frame_processor_base.h (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.h:181: // the extents of |buffer|. On 2014/05/23 17:27:30, acolwell wrote: > This seems a little odd. Is there any reason we can't update |buffer|'s > timestamps before this function is called so we don't have to have these > parameters? This feels like it is leaking details of the caller's implementation > here. wolenetz: Please comment. I think this is reasonable, but it's a larger change in terms of how the frame processor works. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.h:186: // preroll on |buffer| and |audio_preroll_buffer_| will be cleared. On 2014/05/23 19:59:45, wolenetz wrote: > We should also clear |audio_preroll_buffer_| if it exists but does not abut the > next |buffer|, right? Yes, this is done. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.h:190: // timestamp and duration fields will also be set accordingly. On 2014/05/23 19:59:45, wolenetz wrote: > Please clarify that false is returned in this case. This also seems a little > odd/leaking caller's implementation in that the assumption here is that |buffer| > is then expected to be dropped by caller and a subsequent |buffer| is expected > to be given |audio_preroll_buffer_| if it is immediately next. > Comment added. It doesn't matter if it's dropped or not, so it's not a leak. It's unfortunate there's no pass semantics for ref counted buffers here. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.h:218: scoped_refptr<StreamParserBuffer> audio_preroll_buffer_; On 2014/05/23 19:59:45, wolenetz wrote: > nit: Please also clarify the unstated assumption that at most one > |audio_preroll_buffer_| exists across all tracks managed by the frame processor. > If we ever allow multiplexed audio tracks in a stream that supports partial > append window trimming, the assumption would break. That seems unnecessarily verbose given that there's only a single scoped_refptr<StreamParserBuffer> :) https://codereview.chromium.org/276573002/diff/80001/media/filters/legacy_fra... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/legacy_fra... media/filters/legacy_frame_processor.cc:193: // |buffer| has been partially trimmed or had preroll added. On 2014/05/23 19:59:45, wolenetz wrote: > In this case, does this mean that a discontinuity has occurred such that the > track's SourceBufferStream will need to be told that a new media segment has > started, and not always will a preroll buffer have been added to |buffer|? > (I think so, so please add *new_media_segment = true; here) > ditto for track->set_needs_random_access_point(true); > Else broken away, but in any case the random access code was called since HandleXXX() returned false. https://codereview.chromium.org/276573002/diff/80001/media/filters/source_buf... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/source_buf... media/filters/source_buffer_stream.cc:2242: DCHECK(!have_splice_buffers); On 2014/05/23 17:27:30, acolwell wrote: > I think something like DCHECK_NE(have_splice_buffers, have_preroll_buffer); is > closer to what you are actually trying to enforce here. Done.
just a comment reply for now: https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... File media/filters/frame_processor_base.h (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.h:181: // the extents of |buffer|. On 2014/05/23 21:46:26, DaleCurtis wrote: > On 2014/05/23 17:27:30, acolwell wrote: > > This seems a little odd. Is there any reason we can't update |buffer|'s > > timestamps before this function is called so we don't have to have these > > parameters? This feels like it is leaking details of the caller's > implementation > > here. > > wolenetz: Please comment. I think this is reasonable, but it's a larger change > in terms of how the frame processor works. Looking at this now vs our chat yesterday, it looks like the current (new) FrameProcessor code is compatible with setting frame->DTS, frame->PTS and frame->duration prior to the append window trimming (but this must not be done until after the "continue;" line that reprocesses the frame with original PTS/DTS/duration on discontinuity detection). The comments in current code's step 5 will need slight tweaking, and the code subsequent to calling HandlePartialAppendWindowTrimming() will need to be adjusted to use frame-> rather than the locals for PTS/DTS/duration. Probably would be good to update step 9 comments to indicate that frame_end_timestamp remains constant to end to the function (or, hmm, just make it const :))
https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... File media/filters/frame_processor_base.h (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.h:181: // the extents of |buffer|. On 2014/05/23 22:03:48, wolenetz wrote: > On 2014/05/23 21:46:26, DaleCurtis wrote: > > On 2014/05/23 17:27:30, acolwell wrote: > > > This seems a little odd. Is there any reason we can't update |buffer|'s > > > timestamps before this function is called so we don't have to have these > > > parameters? This feels like it is leaking details of the caller's > > implementation > > > here. > > > > wolenetz: Please comment. I think this is reasonable, but it's a larger > change > > in terms of how the frame processor works. > > Looking at this now vs our chat yesterday, it looks like the current (new) > FrameProcessor code is compatible with setting frame->DTS, frame->PTS and > frame->duration prior to the append window trimming (but this must not be done > until after the "continue;" line that reprocesses the frame with original > PTS/DTS/duration on discontinuity detection). The comments in current code's > step 5 will need slight tweaking, and the code subsequent to calling > HandlePartialAppendWindowTrimming() will need to be adjusted to use frame-> > rather than the locals for PTS/DTS/duration. Probably would be good to update > step 9 comments to indicate that frame_end_timestamp remains constant to end to > the function (or, hmm, just make it const :)) Seems like this might be cleaner to do in a follow up patch set then.
https://codereview.chromium.org/276573002/diff/80001/media/filters/chunk_demu... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/chunk_demu... media/filters/chunk_demuxer.cc:571: frame_processor_->clear_audio_preroll_buffer(); On 2014/05/23 21:46:26, DaleCurtis wrote: > On 2014/05/23 19:59:45, wolenetz wrote: > > I would like to see a test that preroll is cleared if config change occurs > prior > > to next buffer even if next buffer abuts the preroll buffer. > > I'll address this in the next patch set. It's proving really tricky to write a > test to do this and I want to get this up for review before acolwell@ heads out > on vacation. Done, but the test fails using the new frame processor, per our offline discussion, since it fails buffers with negative timestamps even if they would be discarded by the append window.
looks pretty good to me. Will leave final approval for Matt. https://codereview.chromium.org/276573002/diff/80001/media/base/stream_parser... File media/base/stream_parser_buffer.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/base/stream_parser... media/base/stream_parser_buffer.cc:179: preroll_buffer_->SetConfigId(GetConfigId()); On 2014/05/23 21:46:26, DaleCurtis wrote: > On 2014/05/23 17:27:30, acolwell wrote: > > Why do you need this? It seems like there would be a problem upstream if these > > got out of sync. I don't think this object really has enough information to be > > sure that this is a safe thing to do. ISTM that you should be asserting that > the > > configID's match instead. > > Because a config ID hasn't yet been assigned to this buffer. That's not done > until SourceBufferStream::Append() by SBS::SetConfigIDs(). ok, but isn't this method always called before then? Both should be unset at this point because neither buffer has reached SBS yet right? What is the situation where the config IDs are different when this method is called? I really don't think this should be calling SetConfigId() here. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor.cc:276: frame_duration = frame->duration(); On 2014/05/23 21:46:26, DaleCurtis wrote: > On 2014/05/23 17:27:30, acolwell wrote: > > Since presentation_timestamp & frame_duration are being updated here, does > > frame_end_timestamp need to be recomputed? > > No, since the timestamp is moved forward by a value equal to that by which the > duration is shrunk. ok. A comment or DCHECK here would be nice so that this is clear. https://codereview.chromium.org/276573002/diff/120001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/276573002/diff/120001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3091: CheckExpectedBuffers(stream, "50 50 60 90 120 150 180 210 240"); Oh. I thought you'd keep the P suffix so you didn't need to have a comment explaining the double timestamp and would still do the discard checks.
Comments addressed. I also fixed the config change + append window test so that it will work without https://codereview.chromium.org/294393002/ needing to land first. https://codereview.chromium.org/276573002/diff/80001/media/base/stream_parser... File media/base/stream_parser_buffer.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/base/stream_parser... media/base/stream_parser_buffer.cc:179: preroll_buffer_->SetConfigId(GetConfigId()); On 2014/05/24 00:58:20, acolwell_OOO_5-24_to_5-30 wrote: > On 2014/05/23 21:46:26, DaleCurtis wrote: > > On 2014/05/23 17:27:30, acolwell wrote: > > > Why do you need this? It seems like there would be a problem upstream if > these > > > got out of sync. I don't think this object really has enough information to > be > > > sure that this is a safe thing to do. ISTM that you should be asserting that > > the > > > configID's match instead. > > > > Because a config ID hasn't yet been assigned to this buffer. That's not done > > until SourceBufferStream::Append() by SBS::SetConfigIDs(). > ok, but isn't this method always called before then? Both should be unset at > this point because neither buffer has reached SBS yet right? What is the > situation where the config IDs are different when this method is called? I > really don't think this should be calling SetConfigId() here. Oh I see what you're saying. Yes this is useless, removed. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor.cc:276: frame_duration = frame->duration(); On 2014/05/24 00:58:20, acolwell_OOO_5-24_to_5-30 wrote: > On 2014/05/23 21:46:26, DaleCurtis wrote: > > On 2014/05/23 17:27:30, acolwell wrote: > > > Since presentation_timestamp & frame_duration are being updated here, does > > > frame_end_timestamp need to be recomputed? > > > > No, since the timestamp is moved forward by a value equal to that by which the > > duration is shrunk. > ok. A comment or DCHECK here would be nice so that this is clear. Done. https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... File media/filters/frame_processor_base.h (right): https://codereview.chromium.org/276573002/diff/80001/media/filters/frame_proc... media/filters/frame_processor_base.h:181: // the extents of |buffer|. On 2014/05/23 22:11:38, DaleCurtis wrote: > On 2014/05/23 22:03:48, wolenetz wrote: > > On 2014/05/23 21:46:26, DaleCurtis wrote: > > > On 2014/05/23 17:27:30, acolwell wrote: > > > > This seems a little odd. Is there any reason we can't update |buffer|'s > > > > timestamps before this function is called so we don't have to have these > > > > parameters? This feels like it is leaking details of the caller's > > > implementation > > > > here. > > > > > > wolenetz: Please comment. I think this is reasonable, but it's a larger > > change > > > in terms of how the frame processor works. > > > > Looking at this now vs our chat yesterday, it looks like the current (new) > > FrameProcessor code is compatible with setting frame->DTS, frame->PTS and > > frame->duration prior to the append window trimming (but this must not be done > > until after the "continue;" line that reprocesses the frame with original > > PTS/DTS/duration on discontinuity detection). The comments in current code's > > step 5 will need slight tweaking, and the code subsequent to calling > > HandlePartialAppendWindowTrimming() will need to be adjusted to use frame-> > > rather than the locals for PTS/DTS/duration. Probably would be good to update > > step 9 comments to indicate that frame_end_timestamp remains constant to end > to > > the function (or, hmm, just make it const :)) > > Seems like this might be cleaner to do in a follow up patch set then. Actually, I found a bug around this, so I've decided to do it in this patch set. Done. https://codereview.chromium.org/276573002/diff/120001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/276573002/diff/120001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3091: CheckExpectedBuffers(stream, "50 50 60 90 120 150 180 210 240"); On 2014/05/24 00:58:21, acolwell_OOO_5-24_to_5-30 wrote: > Oh. I thought you'd keep the P suffix so you didn't need to have a comment > explaining the double timestamp and would still do the discard checks. Done.
looking pretty good. Just a couple comments and some nits: https://codereview.chromium.org/276573002/diff/180001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/276573002/diff/180001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3089: // The first 50ms of the first buffer should be trimmed off since it nit: This comment seems confusing to me. The test looks correct, but perhaps restate comment because we are not actually trimming 50ms off of any buffer. Instead, 50P should be the entirety of the 0K buffer, re-timestamped and marked for full discard. And 50 should be the the 30K buffer with start discard. The net is, yes, 50ms preroll, but across two output buffers from two input buffers. wdyt? https://codereview.chromium.org/276573002/diff/180001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3152: const base::TimeDelta duration_1 = base::TimeDelta::FromMilliseconds(2746); nit: Please add similar TODO(wolenetz): Update this to match the CreateInitDoneCB() init segment's duration expectation once the files are updated to have consistent duration in init segment. See http://crbug.com/354284. https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... media/filters/frame_processor.cc:165: // Note: |frame| PTS is only updated if it survives processing. nit: s/survives/survives discontinuity/ https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... media/filters/frame_processor.cc:169: // Frame DTS is only updated if it survives processing. nit: ditto https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... media/filters/frame_processor.cc:273: // |frame| has been partially trimmed or had preroll added. I agree that the false path would set the flags for new media segment/random access point needed, but in the case where no |audio_preroll_buffer_| was previously held aside, and |frame| overlaps |append_window_start| and therefore has been partially trimmed, we should still signal SBS of a new media segment (if not in sequence mode). We shouldn't need to worry about setting needs_random_access_point in this case since HandlePartialAppendWindowTrimming DCHECKS the partially trimmed frame is a key frame. Net: Please add the following here, perhaps with a clarifying comment. if (!sequence_mode_) *new_media_segment = true; https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... media/filters/frame_processor.cc:337: track_buffer->set_last_frame_duration(frame_duration); fyi: A side effect of partial frame discard is that the updated (shorter) track_buffer->last_frame_duration() will make subsequent buffer append that much more sensitive to possible discontinuity, it it does not immediately abut frame_end_timestamp. I doubt this will cause any big problem though. https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... File media/filters/frame_processor_base.h (right): https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... media/filters/frame_processor_base.h:186: // normally be discarded, |audio_preroll_buffer_| will be set to |buffer|; its nit: nix the "; its timestamp..accordingly." since setting |audio_preroll_buffer_| to |buffer| does this. https://codereview.chromium.org/276573002/diff/180001/media/filters/legacy_fr... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/180001/media/filters/legacy_fr... media/filters/legacy_frame_processor.cc:190: // |buffer| has been partially trimmed or had preroll added. Ditto of my comment in PS6 frame_processor.cc: Please include here with some comment: *new_media_segment = true; (LegacyFrameProcessor doesn't support sequence mode, so no condition needed here for !sequence_mode.)
Some further comments from offline chats. acolwell@, am I correct that sequence mode append window filtering (even without partial discards, just full discards) could introduce gaps? https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... media/filters/frame_processor.cc:273: // |frame| has been partially trimmed or had preroll added. On 2014/05/30 19:58:45, wolenetz wrote: > I agree that the false path would set the flags for new media segment/random > access point needed, but in the case where no |audio_preroll_buffer_| was > previously held aside, and |frame| overlaps |append_window_start| and therefore > has been partially trimmed, we should still signal SBS of a new media segment > (if not in sequence mode). We shouldn't need to worry about setting > needs_random_access_point in this case since HandlePartialAppendWindowTrimming > DCHECKS the partially trimmed frame is a key frame. Net: Please add the > following here, perhaps with a clarifying comment. > if (!sequence_mode_) > *new_media_segment = true; From our chat, it's also possible that append window partial discard even in sequence mode could introduce a gap such that SBS should be told a new media segment is starting. Please add: (here and also in LegacyFrameProcessor) if (frame->timestamp() != presentation_timestamp) { *new_media_segment = true; track_buffer->set_needs_random_access_point(true); } https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... media/filters/frame_processor.cc:287: if (!sequence_mode_) { From our chat, this condition needs to be dropped. Even in sequence mode, a gap could be introduced. If MSE spec editors truly desire sequence mode append window filtering to filter timestampOffset-adjusted-appended-buffers such that any buffers surviving the filter get further relocated (and timestampOffset further adjusted) such that they start without gap after previous group_end_timestamp, then I believe the spec would need updating. acolwell@, please comment on this.
https://codereview.chromium.org/276573002/diff/180001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/276573002/diff/180001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3089: // The first 50ms of the first buffer should be trimmed off since it On 2014/05/30 19:58:45, wolenetz wrote: > nit: This comment seems confusing to me. The test looks correct, but perhaps > restate comment because we are not actually trimming 50ms off of any buffer. > Instead, 50P should be the entirety of the 0K buffer, re-timestamped and marked > for full discard. And 50 should be the the 30K buffer with start discard. The > net is, yes, 50ms preroll, but across two output buffers from two input buffers. > wdyt? acolwell@ suggested my previous comment to that effect be removed. I agree it's confusing though, so I've put it back with some edits. https://codereview.chromium.org/276573002/diff/180001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3152: const base::TimeDelta duration_1 = base::TimeDelta::FromMilliseconds(2746); On 2014/05/30 19:58:45, wolenetz wrote: > nit: Please add similar TODO(wolenetz): Update this to match the > CreateInitDoneCB() init segment's duration expectation once the files are > updated to have consistent duration in init segment. See > http://crbug.com/354284. Done. https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... media/filters/frame_processor.cc:165: // Note: |frame| PTS is only updated if it survives processing. On 2014/05/30 19:58:45, wolenetz wrote: > nit: s/survives/survives discontinuity/ Done. https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... media/filters/frame_processor.cc:169: // Frame DTS is only updated if it survives processing. On 2014/05/30 19:58:45, wolenetz wrote: > nit: ditto Done. https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... media/filters/frame_processor.cc:273: // |frame| has been partially trimmed or had preroll added. On 2014/05/30 20:45:12, wolenetz wrote: > On 2014/05/30 19:58:45, wolenetz wrote: > > I agree that the false path would set the flags for new media segment/random > > access point needed, but in the case where no |audio_preroll_buffer_| was > > previously held aside, and |frame| overlaps |append_window_start| and > therefore > > has been partially trimmed, we should still signal SBS of a new media segment > > (if not in sequence mode). We shouldn't need to worry about setting > > needs_random_access_point in this case since HandlePartialAppendWindowTrimming > > DCHECKS the partially trimmed frame is a key frame. Net: Please add the > > following here, perhaps with a clarifying comment. > > if (!sequence_mode_) > > *new_media_segment = true; > > From our chat, it's also possible that append window partial discard even in > sequence mode could introduce a gap such that SBS should be told a new media > segment is starting. > Please add: (here and also in LegacyFrameProcessor) > if (frame->timestamp() != presentation_timestamp) { > *new_media_segment = true; > track_buffer->set_needs_random_access_point(true); > } Done. https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... media/filters/frame_processor.cc:287: if (!sequence_mode_) { On 2014/05/30 20:45:12, wolenetz wrote: > From our chat, this condition needs to be dropped. Even in sequence mode, a gap > could be introduced. > > If MSE spec editors truly desire sequence mode append window filtering to filter > timestampOffset-adjusted-appended-buffers such that any buffers surviving the > filter get further relocated (and timestampOffset further adjusted) such that > they start without gap after previous group_end_timestamp, then I believe the > spec would need updating. > > acolwell@, please comment on this. I defer to you guys to sort this out outside of this patch. https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... File media/filters/frame_processor_base.h (right): https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... media/filters/frame_processor_base.h:186: // normally be discarded, |audio_preroll_buffer_| will be set to |buffer|; its On 2014/05/30 19:58:45, wolenetz wrote: > nit: nix the "; its timestamp..accordingly." since setting > |audio_preroll_buffer_| to |buffer| does this. Done. https://codereview.chromium.org/276573002/diff/180001/media/filters/legacy_fr... File media/filters/legacy_frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/180001/media/filters/legacy_fr... media/filters/legacy_frame_processor.cc:190: // |buffer| has been partially trimmed or had preroll added. On 2014/05/30 19:58:45, wolenetz wrote: > Ditto of my comment in PS6 frame_processor.cc: > Please include here with some comment: > *new_media_segment = true; > (LegacyFrameProcessor doesn't support sequence mode, so no condition needed here > for !sequence_mode.) Done.
lgtm % nit https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... media/filters/frame_processor.cc:287: if (!sequence_mode_) { On 2014/05/30 20:54:30, DaleCurtis wrote: > On 2014/05/30 20:45:12, wolenetz wrote: > > From our chat, this condition needs to be dropped. Even in sequence mode, a > gap > > could be introduced. > > > > If MSE spec editors truly desire sequence mode append window filtering to > filter > > timestampOffset-adjusted-appended-buffers such that any buffers surviving the > > filter get further relocated (and timestampOffset further adjusted) such that > > they start without gap after previous group_end_timestamp, then I believe the > > spec would need updating. > > > > acolwell@, please comment on this. > > I defer to you guys to sort this out outside of this patch. SGTM. On further inspection, and per our offline chat, leaving this as-is conforms to the intent of sequence mode. I retract my earlier comment here and am working on a separate CL that clarifies new frame processor behavior w.r.t. small append window discards. https://codereview.chromium.org/276573002/diff/210001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/276573002/diff/210001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3155: // TODO(wolenetz/acolwell): Remove this extra SetDuration expectation once nit: s/Remove this extra SetDuration/Update this duration/
Thanks for review! https://codereview.chromium.org/276573002/diff/210001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/276573002/diff/210001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:3155: // TODO(wolenetz/acolwell): Remove this extra SetDuration expectation once On 2014/05/30 22:31:42, wolenetz wrote: > nit: s/Remove this extra SetDuration/Update this duration/ Done.
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/276573002/230001
https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/276573002/diff/180001/media/filters/frame_pro... media/filters/frame_processor.cc:287: if (!sequence_mode_) { On 2014/05/30 22:31:42, wolenetz wrote: > On 2014/05/30 20:54:30, DaleCurtis wrote: > > On 2014/05/30 20:45:12, wolenetz wrote: > > > From our chat, this condition needs to be dropped. Even in sequence mode, a > > gap > > > could be introduced. > > > > > > If MSE spec editors truly desire sequence mode append window filtering to > > filter > > > timestampOffset-adjusted-appended-buffers such that any buffers surviving > the > > > filter get further relocated (and timestampOffset further adjusted) such > that > > > they start without gap after previous group_end_timestamp, then I believe > the > > > spec would need updating. > > > > > > acolwell@, please comment on this. > > > > I defer to you guys to sort this out outside of this patch. > > SGTM. On further inspection, and per our offline chat, leaving this as-is > conforms to the intent of sequence mode. I retract my earlier comment here and > am working on a separate CL that clarifies new frame processor behavior w.r.t. > small append window discards. I have a separate CL to test/clarify small append window discards: https://codereview.chromium.org/310483005/
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/276573002/250001
Message was sent while issue was closed.
Committed patchset #10 manually as r274073 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
