Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
414603002:
Add support for partial append window end trimming. (Closed)
|
Created:
6 years, 5 months ago by DaleCurtis Modified:
6 years, 4 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd support for partial append window end trimming.
Facilitates gapless playback across mp3 and aac. Relying on splice
frames and crossfading doesn't work in cases where the prior segment
signal diverges to null-padding values too quickly relative to the
newly appended segment.
BUG=395899
TEST=new unittests. llama-demo works.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287174
Patch Set 1 : Typos. #
Total comments: 21
Patch Set 2 : Fixes. #
Total comments: 26
Patch Set 3 : Comments. #
Total comments: 19
Patch Set 4 : Comments. #
Messages
Total messages: 18 (0 generated)
https://codereview.chromium.org/414603002/diff/20001/media/base/audio_discard... File media/base/audio_discard_helper.cc (right): https://codereview.chromium.org/414603002/diff/20001/media/base/audio_discard... media/base/audio_discard_helper.cc:203: DCHECK_LT(decoder_delay_, original_frame_count); Will this blow up on Opus & Vorbis then? https://codereview.chromium.org/414603002/diff/20001/media/base/audio_discard... media/base/audio_discard_helper.cc:210: end_frames_to_discard = end_frames_to_discard - decoder_delay_; nit: -= here? https://codereview.chromium.org/414603002/diff/20001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/414603002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1376: MuxedStreamInfo(kAudioTrackNum, "0 23K"), nit: Remove 0 and s/23K/23D23K/ . The intent of this part of the test was to verify that the initial non-keyframes would get dropped. https://codereview.chromium.org/414603002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1379: CheckExpectedRanges(kSourceId, "{ [0,46) }"); This shouldn't be needed after the change above. https://codereview.chromium.org/414603002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1383: MuxedStreamInfo(kAudioTrackNum, "46 69K"), nit: s/46/46K/ just so it is explicit please. https://codereview.chromium.org/414603002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1386: CheckExpectedRanges(kSourceId, "{ [0,92) }"); ditto https://codereview.chromium.org/414603002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1388: CheckExpectedBuffers(audio_stream, "0 23"); I think this should be "23 46 69" since everything is a keyframe now. https://codereview.chromium.org/414603002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:3362: CheckExpectedRanges(kSourceId, "{ [50,280) }"); why did this and the range below change? https://codereview.chromium.org/414603002/diff/20001/media/filters/frame_proc... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/414603002/diff/20001/media/filters/frame_proc... media/filters/frame_processor.cc:403: if (buffer->timestamp() <= append_window_end && s/<=/</ ? How do you get into the == situation? It seems like those buffers shouldn't even get here. What am I forgetting here? https://codereview.chromium.org/414603002/diff/20001/media/filters/frame_proc... media/filters/frame_processor.cc:608: // here, so |track_buffer|'s last frame duration update uses original Is this still true since you are setting a new frame_end_timestamp now?
https://codereview.chromium.org/414603002/diff/20001/media/base/audio_discard... File media/base/audio_discard_helper.cc (right): https://codereview.chromium.org/414603002/diff/20001/media/base/audio_discard... media/base/audio_discard_helper.cc:203: DCHECK_LT(decoder_delay_, original_frame_count); On 2014/07/23 00:15:42, acolwell wrote: > Will this blow up on Opus & Vorbis then? No, they don't have any decoder delay; only MP3 does. https://codereview.chromium.org/414603002/diff/20001/media/base/audio_discard... media/base/audio_discard_helper.cc:210: end_frames_to_discard = end_frames_to_discard - decoder_delay_; On 2014/07/23 00:15:41, acolwell wrote: > nit: -= here? Done. https://codereview.chromium.org/414603002/diff/20001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/414603002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1376: MuxedStreamInfo(kAudioTrackNum, "0 23K"), On 2014/07/23 00:15:42, acolwell wrote: > nit: Remove 0 and s/23K/23D23K/ . The intent of this part of the test was to > verify that the initial non-keyframes would get dropped. Doesn't look like "D" is an understood code here. I switched it to 23K 43K, is that okay? https://codereview.chromium.org/414603002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1379: CheckExpectedRanges(kSourceId, "{ [0,46) }"); On 2014/07/23 00:15:42, acolwell wrote: > This shouldn't be needed after the change above. Acknowledged. https://codereview.chromium.org/414603002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1383: MuxedStreamInfo(kAudioTrackNum, "46 69K"), On 2014/07/23 00:15:42, acolwell wrote: > nit: s/46/46K/ just so it is explicit please. Done. https://codereview.chromium.org/414603002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1386: CheckExpectedRanges(kSourceId, "{ [0,92) }"); On 2014/07/23 00:15:42, acolwell wrote: > ditto Acknowledged. https://codereview.chromium.org/414603002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1388: CheckExpectedBuffers(audio_stream, "0 23"); On 2014/07/23 00:15:42, acolwell wrote: > I think this should be "23 46 69" since everything is a keyframe now. 23 43 with my change above. https://codereview.chromium.org/414603002/diff/20001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:3362: CheckExpectedRanges(kSourceId, "{ [50,280) }"); On 2014/07/23 00:15:42, acolwell wrote: > why did this and the range below change? Previously 270K was being entirely discarded since it overlapped the append window end, now there's a partial discard of it. Ditto for everything below. https://codereview.chromium.org/414603002/diff/20001/media/filters/frame_proc... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/414603002/diff/20001/media/filters/frame_proc... media/filters/frame_processor.cc:403: if (buffer->timestamp() <= append_window_end && On 2014/07/23 00:15:42, acolwell wrote: > s/<=/</ ? How do you get into the == situation? It seems like those buffers > shouldn't even get here. What am I forgetting here? You're right, <= should have been skipped in the if on l.340. Fixed. https://codereview.chromium.org/414603002/diff/20001/media/filters/frame_proc... media/filters/frame_processor.cc:608: // here, so |track_buffer|'s last frame duration update uses original On 2014/07/23 00:15:42, acolwell wrote: > Is this still true since you are setting a new frame_end_timestamp now? I'm not sure, wolenetz?
Ping?
Here are a few more comments. I'd really like wolenetz to weigh in on this as well before landing it. https://codereview.chromium.org/414603002/diff/40001/media/base/audio_discard... File media/base/audio_discard_helper.cc (right): https://codereview.chromium.org/414603002/diff/40001/media/base/audio_discard... media/base/audio_discard_helper.cc:212: delayed_end_discard_ = decoder_delay_ - end_frames_to_discard; Can you just store end_frames_to_discard here and then rework the TrimRange() computation in ProcessBuffers(). ISTM that would be easier to understand and make the comment for this member variable less confusing. IIUC this is just delaying the end discard amount until we receive the next buffer. https://codereview.chromium.org/414603002/diff/40001/media/base/audio_discard... File media/base/audio_discard_helper_unittest.cc (right): https://codereview.chromium.org/414603002/diff/40001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:336: // within the next decoded buffer. I'm confused by this comment. Wasn't the decoder delay handled by the call above. I thought we didn't get data for the first buffer because the first half was discarded because of the discard_padding and the second half was discarded because of the decoding delay. Since the code below is specifying discarding half of the beginning and one quarter at the end, shouldn't that mean I should expect a kDuration/4 buffer to be returned? I'm really having a hard time visualizing how this test is supposed to be working. https://codereview.chromium.org/414603002/diff/40001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:355: encoded_buffer->set_timestamp(kTimestamp + kDuration); Shouldn't you be using + 2 * kDuration here so it doesn't overlap the buffer added above? https://codereview.chromium.org/414603002/diff/40001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:372: encoded_buffer->set_timestamp(kTimestamp + kDuration * 2); Shouldn't this be *3? https://codereview.chromium.org/414603002/diff/40001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/414603002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1376: MuxedStreamInfo(kAudioTrackNum, "23K 43K"), do you actually need this second buffer? Ideally it would be nice to not have this buffer overlap with the one added below just like the original test. https://codereview.chromium.org/414603002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1388: CheckExpectedBuffers(audio_stream, "23 43"); This should include buffers from the second append just like the original code did. Assuming you removed the 43 from avove, I'd expect you to be able to make this "23 46 69". If that doesn't work, it would be nice to know why.
https://codereview.chromium.org/414603002/diff/40001/media/base/audio_discard... File media/base/audio_discard_helper_unittest.cc (right): https://codereview.chromium.org/414603002/diff/40001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:336: // within the next decoded buffer. On 2014/07/24 19:22:14, acolwell wrote: > I'm confused by this comment. Wasn't the decoder delay handled by the call > above. I thought we didn't get data for the first buffer because the first half > was discarded because of the discard_padding and the second half was discarded > because of the decoding delay. > > Since the code below is specifying discarding half of the beginning and one > quarter at the end, shouldn't that mean I should expect a kDuration/4 buffer to > be returned? > > I'm really having a hard time visualizing how this test is supposed to be > working. Decoder delay is forever. Remember it's not included in the timeline, so all future buffers must be adjusted to compensate.
https://codereview.chromium.org/414603002/diff/20001/media/filters/frame_proc... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/414603002/diff/20001/media/filters/frame_proc... media/filters/frame_processor.cc:608: // here, so |track_buffer|'s last frame duration update uses original On 2014/07/23 01:50:41, DaleCurtis wrote: > On 2014/07/23 00:15:42, acolwell wrote: > > Is this still true since you are setting a new frame_end_timestamp now? > > I'm not sure, wolenetz? I believe this is still correct. |frame_duration| should not be changed here; it is used, below, to set track_buffer's last frame duration, and in later iterations of this method, that "last frame duration" is used to see if there is a large enough gap since "last decode timestamp" to trigger discontinuity logic. Append window trimming correctly reduces the trimmed buffer's duration, but does not reduce the eventual track buffer's "last frame duration". This seems to me to be a quality of implementation issue, where spurious discontinuity detections were problematic, with "fix" being to use the original |frame_duration| if the frame survives and is appended to the track buffer. https://codereview.chromium.org/414603002/diff/40001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/414603002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:462: block_info.flags = kWebMFlagKeyframe; nit: assert that block_info.flags == kWebMFlagKeyframe here, rather than implicitly setting this flag, to make it obvious to test maintainers that (test) audio streams must consist entirely of key frames. Also, perhaps document this restriction in method's comment. https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... media/filters/frame_processor.cc:342: (buffer->timestamp() > append_window_start && Is there any allowance we need to make here for applying a previously saved audio_preroll_buffer_ to a buffer that starts slightly after append_window_start? ISTM we need to allow variance within +/- |sample_duration_| here rather than always assuming there is no partial append window processing needed here if buffer->timestamp() is strictly > |append_window_start_|. Also, this looks like it needs some unit test coverage; please add. https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... media/filters/frame_processor.cc:363: if (buffer->timestamp() <= append_window_start) { ditto https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... media/filters/frame_processor.cc:601: // If |frame| was shortened a discontinuity may exist, so treat the next s/was shortened/was front-trimmed/ https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... media/filters/frame_processor.cc:612: frame_end_timestamp = frame->timestamp() + frame->duration(); nit: Though we currently require all paths that reach here to be due to keyframed buffer streams, it would be good to either comment, or even better require in code that track_buffer->set_needs_random_access_point(true) is called if we reach this point in this code block. See the next code block, below, that would do this except |presentation_timestamp| and |frame_end_timestamp| are clamped to the append window in this code path here.
https://codereview.chromium.org/414603002/diff/40001/media/base/audio_discard... File media/base/audio_discard_helper.cc (right): https://codereview.chromium.org/414603002/diff/40001/media/base/audio_discard... media/base/audio_discard_helper.cc:212: delayed_end_discard_ = decoder_delay_ - end_frames_to_discard; On 2014/07/24 19:22:14, acolwell wrote: > Can you just store end_frames_to_discard here and then rework the TrimRange() > computation in ProcessBuffers(). ISTM that would be easier to understand and > make the comment for this member variable less confusing. IIUC this is just > delaying the end discard amount until we receive the next buffer. Done. https://codereview.chromium.org/414603002/diff/40001/media/base/audio_discard... File media/base/audio_discard_helper_unittest.cc (right): https://codereview.chromium.org/414603002/diff/40001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:336: // within the next decoded buffer. On 2014/07/24 19:58:59, DaleCurtis wrote: > On 2014/07/24 19:22:14, acolwell wrote: > > I'm confused by this comment. Wasn't the decoder delay handled by the call > > above. I thought we didn't get data for the first buffer because the first > half > > was discarded because of the discard_padding and the second half was discarded > > because of the decoding delay. > > > > Since the code below is specifying discarding half of the beginning and one > > quarter at the end, shouldn't that mean I should expect a kDuration/4 buffer > to > > be returned? > > > > I'm really having a hard time visualizing how this test is supposed to be > > working. > > Decoder delay is forever. Remember it's not included in the timeline, so all > future buffers must be adjusted to compensate. Per our offline conversation I've added diagrams explaining exactly what's happening here. https://codereview.chromium.org/414603002/diff/40001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:355: encoded_buffer->set_timestamp(kTimestamp + kDuration); On 2014/07/24 19:22:14, acolwell wrote: > Shouldn't you be using + 2 * kDuration here so it doesn't overlap the buffer > added above? Done. https://codereview.chromium.org/414603002/diff/40001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:372: encoded_buffer->set_timestamp(kTimestamp + kDuration * 2); On 2014/07/24 19:22:14, acolwell wrote: > Shouldn't this be *3? Done. https://codereview.chromium.org/414603002/diff/40001/media/filters/chunk_demu... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/414603002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:462: block_info.flags = kWebMFlagKeyframe; On 2014/07/25 22:33:03, wolenetz wrote: > nit: assert that block_info.flags == kWebMFlagKeyframe here, rather than > implicitly setting this flag, to make it obvious to test maintainers that (test) > audio streams must consist entirely of key frames. Also, perhaps document this > restriction in method's comment. Looking at this again it looks like only a few tests need fixup, so I've gone ahead and fixed them and added the assert here. https://codereview.chromium.org/414603002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1376: MuxedStreamInfo(kAudioTrackNum, "23K 43K"), On 2014/07/24 19:22:14, acolwell wrote: > do you actually need this second buffer? Ideally it would be nice to not have > this buffer overlap with the one added below just like the original test. Done. https://codereview.chromium.org/414603002/diff/40001/media/filters/chunk_demu... media/filters/chunk_demuxer_unittest.cc:1388: CheckExpectedBuffers(audio_stream, "23 43"); On 2014/07/24 19:22:15, acolwell wrote: > This should include buffers from the second append just like the original code > did. Assuming you removed the 43 from avove, I'd expect you to be able to make > this "23 46 69". If that doesn't work, it would be nice to know why. Done. https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... media/filters/frame_processor.cc:342: (buffer->timestamp() > append_window_start && On 2014/07/25 22:33:03, wolenetz wrote: > Is there any allowance we need to make here for applying a previously saved > audio_preroll_buffer_ to a buffer that starts slightly after > append_window_start? ISTM we need to allow variance within +/- > |sample_duration_| here rather than always assuming there is no partial append > window processing needed here if buffer->timestamp() is strictly > > |append_window_start_|. > Also, this looks like it needs some unit test coverage; please add. We don't attach preroll for disjoint buffers. If a buffer started after append_window_start there'd be a gap between any buffer which was before it. Otherwise we'd have already processed that buffer for partial discard and attached any preroll to it. That said, we were missing cases where the preroll was within range for use around the append window start. I've reflowed the code for legibility in this regard. https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... media/filters/frame_processor.cc:363: if (buffer->timestamp() <= append_window_start) { On 2014/07/25 22:33:03, wolenetz wrote: > ditto Acknowledged. https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... media/filters/frame_processor.cc:601: // If |frame| was shortened a discontinuity may exist, so treat the next On 2014/07/25 22:33:03, wolenetz wrote: > s/was shortened/was front-trimmed/ Done. https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... media/filters/frame_processor.cc:612: frame_end_timestamp = frame->timestamp() + frame->duration(); On 2014/07/25 22:33:04, wolenetz wrote: > nit: Though we currently require all paths that reach here to be due to > keyframed buffer streams, it would be good to either comment, or even better > require in code that track_buffer->set_needs_random_access_point(true) is called > if we reach this point in this code block. See the next code block, below, that > would do this except |presentation_timestamp| and |frame_end_timestamp| are > clamped to the append window in this code path here. Are you saying we should call that if we trim the buffer in any way? Or always?
looking pretty good https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... media/filters/frame_processor.cc:612: frame_end_timestamp = frame->timestamp() + frame->duration(); On 2014/07/29 01:35:10, DaleCurtis wrote: > On 2014/07/25 22:33:04, wolenetz wrote: > > nit: Though we currently require all paths that reach here to be due to > > keyframed buffer streams, it would be good to either comment, or even better > > require in code that track_buffer->set_needs_random_access_point(true) is > called > > if we reach this point in this code block. See the next code block, below, > that > > would do this except |presentation_timestamp| and |frame_end_timestamp| are > > clamped to the append window in this code path here. > > Are you saying we should call that if we trim the buffer in any way? Or always? I see. If just preroll added, we don't need to call set_needs_random_access_point(true) on track_buffer. But if trimmed in any way, we should (though for keyframed streams it I think is just overhead). https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... File media/base/audio_discard_helper.h (right): https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... media/base/audio_discard_helper.h:68: size_t discard_frames_; nit: Please document these members that are missing docs, to help future CR of changes to this class. https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... File media/base/audio_discard_helper_unittest.cc (right): https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:340: // Processing another buffer should discard the back half of the buffer since nit: s/another buffer should/another buffer that has front discard set to half the buffer's duration should/ https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:341: // kDecoderDelay is half a buffer. The end padding should not be discarded nit: s/since kDecoderDelay is half a buffer/and front discard should start at index kDecoderDelay, which is half a buffer/ https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:360: kDataStep * 1000); This nearness distance requirement seems too loose. kDecoderDelay is 48000 / 100 / 2, which is 240. 1000 >> 240. ISTM we are allowing for the midpoint value of the created buffer to be off by a factor of about 4 whole buffers worth of data. Or am I missing sth obvious here? Remove the "* 1000" here? https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:370: // kDuration / 2 so that the next buffer has its start entirely discarded. nit: this "Use kDuration / 2" portion of the comment is confusing. Use it for what? Which next buffer are we talking about vs the previous sentence? https://codereview.chromium.org/414603002/diff/60001/media/filters/frame_proc... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/414603002/diff/60001/media/filters/frame_proc... media/filters/frame_processor.cc:330: base::TimeDelta append_window_start, nit: Update this method's documentation in the .h (w.r.t. end trimming). https://codereview.chromium.org/414603002/diff/60001/media/filters/frame_proc... media/filters/frame_processor.cc:349: if (buffer->timestamp() > append_window_end) s/>/>=. append_window_start cannot == append_window_end per spec. So buffer is strictly outside append window even if it starts at the append window end time. https://codereview.chromium.org/414603002/diff/60001/media/filters/frame_proc... media/filters/frame_processor.cc:369: buffer->SetPrerollBuffer(audio_preroll_buffer_); We need to return true later in this case (per method comment in .h). Suggest s/trimmed_buffer/buffer_trimmed_or_preroll_added/. Suggest also adding a test that exposes the scenario "preroll successfully added to buffer that is entirely within append window", or did one exist already that failed for this PS? https://codereview.chromium.org/414603002/diff/60001/media/filters/frame_proc... media/filters/frame_processor.cc:398: if (buffer->timestamp() < append_window_end && nit: drop (or change to a DCHECK) the first clause here, once line 349 is changed to >=.
https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... File media/base/audio_discard_helper.h (right): https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... media/base/audio_discard_helper.h:68: size_t discard_frames_; On 2014/07/30 22:04:31, wolenetz wrote: > nit: Please document these members that are missing docs, to help future CR of > changes to this class. Done. https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... File media/base/audio_discard_helper_unittest.cc (right): https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:340: // Processing another buffer should discard the back half of the buffer since On 2014/07/30 22:04:31, wolenetz wrote: > nit: s/another buffer should/another buffer that has front discard set to half > the buffer's duration should/ Done. https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:341: // kDecoderDelay is half a buffer. The end padding should not be discarded On 2014/07/30 22:04:32, wolenetz wrote: > nit: s/since kDecoderDelay is half a buffer/and front discard should start at > index kDecoderDelay, which is half a buffer/ I don't think this one is helpful and makes the sentence even more of a run on. https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:360: kDataStep * 1000); On 2014/07/30 22:04:31, wolenetz wrote: > This nearness distance requirement seems too loose. kDecoderDelay is 48000 / 100 > / 2, which is 240. 1000 >> 240. ISTM we are allowing for the midpoint value of > the created buffer to be off by a factor of about 4 whole buffers worth of data. > Or am I missing sth obvious here? Remove the "* 1000" here? Whoops, this should be kDataStep / 1000 meaning the accuracy needs to be 0.000010f. https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:370: // kDuration / 2 so that the next buffer has its start entirely discarded. On 2014/07/30 22:04:31, wolenetz wrote: > nit: this "Use kDuration / 2" portion of the comment is confusing. Use it for > what? Which next buffer are we talking about vs the previous sentence? As the end discard padding. I've clarified the sentence. https://codereview.chromium.org/414603002/diff/60001/media/filters/frame_proc... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/414603002/diff/60001/media/filters/frame_proc... media/filters/frame_processor.cc:330: base::TimeDelta append_window_start, On 2014/07/30 22:04:32, wolenetz wrote: > nit: Update this method's documentation in the .h (w.r.t. end trimming). Done. https://codereview.chromium.org/414603002/diff/60001/media/filters/frame_proc... media/filters/frame_processor.cc:349: if (buffer->timestamp() > append_window_end) On 2014/07/30 22:04:32, wolenetz wrote: > s/>/>=. append_window_start cannot == append_window_end per spec. So buffer is > strictly outside append window even if it starts at the append window end time. Done. https://codereview.chromium.org/414603002/diff/60001/media/filters/frame_proc... media/filters/frame_processor.cc:369: buffer->SetPrerollBuffer(audio_preroll_buffer_); On 2014/07/30 22:04:32, wolenetz wrote: > We need to return true later in this case (per method comment in .h). Suggest > s/trimmed_buffer/buffer_trimmed_or_preroll_added/. Suggest also adding a test > that exposes the scenario "preroll successfully added to buffer that is entirely > within append window", or did one exist already that failed for this PS? Done. Doesn't matter though since the buffer would pass the existing append window checks. https://codereview.chromium.org/414603002/diff/60001/media/filters/frame_proc... media/filters/frame_processor.cc:398: if (buffer->timestamp() < append_window_end && On 2014/07/30 22:04:32, wolenetz wrote: > nit: drop (or change to a DCHECK) the first clause here, once line 349 is > changed to >=. Done.
I have just one comment needing action still (from PS2, actually...): https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... media/filters/frame_processor.cc:612: frame_end_timestamp = frame->timestamp() + frame->duration(); On 2014/07/30 22:04:31, wolenetz wrote: > On 2014/07/29 01:35:10, DaleCurtis wrote: > > On 2014/07/25 22:33:04, wolenetz wrote: > > > nit: Though we currently require all paths that reach here to be due to > > > keyframed buffer streams, it would be good to either comment, or even better > > > require in code that track_buffer->set_needs_random_access_point(true) is > > called > > > if we reach this point in this code block. See the next code block, below, > > that > > > would do this except |presentation_timestamp| and |frame_end_timestamp| are > > > clamped to the append window in this code path here. > > > > Are you saying we should call that if we trim the buffer in any way? Or > always? > > I see. If just preroll added, we don't need to call > set_needs_random_access_point(true) on track_buffer. But if trimmed in any way, > we should (though for keyframed streams it I think is just overhead). I think my earlier reply to this comment was missed. https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... File media/base/audio_discard_helper_unittest.cc (right): https://codereview.chromium.org/414603002/diff/60001/media/base/audio_discard... media/base/audio_discard_helper_unittest.cc:341: // kDecoderDelay is half a buffer. The end padding should not be discarded On 2014/07/30 22:52:00, DaleCurtis wrote: > On 2014/07/30 22:04:32, wolenetz wrote: > > nit: s/since kDecoderDelay is half a buffer/and front discard should start at > > index kDecoderDelay, which is half a buffer/ > > I don't think this one is helpful and makes the sentence even more of a run on. Acknowledged.
https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... File media/filters/frame_processor.cc (right): https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... media/filters/frame_processor.cc:612: frame_end_timestamp = frame->timestamp() + frame->duration(); On 2014/07/30 23:12:34, wolenetz wrote: > On 2014/07/30 22:04:31, wolenetz wrote: > > On 2014/07/29 01:35:10, DaleCurtis wrote: > > > On 2014/07/25 22:33:04, wolenetz wrote: > > > > nit: Though we currently require all paths that reach here to be due to > > > > keyframed buffer streams, it would be good to either comment, or even > better > > > > require in code that track_buffer->set_needs_random_access_point(true) is > > > called > > > > if we reach this point in this code block. See the next code block, below, > > > that > > > > would do this except |presentation_timestamp| and |frame_end_timestamp| > are > > > > clamped to the append window in this code path here. > > > > > > Are you saying we should call that if we trim the buffer in any way? Or > > always? > > > > I see. If just preroll added, we don't need to call > > set_needs_random_access_point(true) on track_buffer. But if trimmed in any > way, > > we should (though for keyframed streams it I think is just overhead). > > I think my earlier reply to this comment was missed. Whoops, I didn't see this, though I'm not sure I follow. Can you elaborate or why this should be called here? It's required via DCHECK that all frames trimmed are keyframes. Partial append window trimming all happens post decode, so it would never change the frame type anyways. To me it seems you only want to call set_needs_random_access_point() if you are completely dropping frames.
On 2014/07/31 00:00:32, DaleCurtis wrote: > https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... > File media/filters/frame_processor.cc (right): > > https://codereview.chromium.org/414603002/diff/40001/media/filters/frame_proc... > media/filters/frame_processor.cc:612: frame_end_timestamp = frame->timestamp() + > frame->duration(); > On 2014/07/30 23:12:34, wolenetz wrote: > > On 2014/07/30 22:04:31, wolenetz wrote: > > > On 2014/07/29 01:35:10, DaleCurtis wrote: > > > > On 2014/07/25 22:33:04, wolenetz wrote: > > > > > nit: Though we currently require all paths that reach here to be due to > > > > > keyframed buffer streams, it would be good to either comment, or even > > better > > > > > require in code that track_buffer->set_needs_random_access_point(true) > is > > > > called > > > > > if we reach this point in this code block. See the next code block, > below, > > > > that > > > > > would do this except |presentation_timestamp| and |frame_end_timestamp| > > are > > > > > clamped to the append window in this code path here. > > > > > > > > Are you saying we should call that if we trim the buffer in any way? Or > > > always? > > > > > > I see. If just preroll added, we don't need to call > > > set_needs_random_access_point(true) on track_buffer. But if trimmed in any > > way, > > > we should (though for keyframed streams it I think is just overhead). > > > > I think my earlier reply to this comment was missed. > > Whoops, I didn't see this, though I'm not sure I follow. Can you elaborate or > why this should be called here? It's required via DCHECK that all frames trimmed > are keyframes. Partial append window trimming all happens post decode, so it > would never change the frame type anyways. > > To me it seems you only want to call set_needs_random_access_point() if you are > completely dropping frames. The current code is technically correct for at least debug builds. My comment was more around future-proofing/not making a 'fully keyframed stream' assumption within the frame processor's handling of retval from partial frame trimming. If we ever changed logic to allow non-fully-keyframed streams, we might forget. As such, I have no strong feeling, and it's really just a nit. sorry. So, PS4 lgtm % maybe adding a comment in code about this.
lgtm
Thanks for review. I think the DCHECK plus the inherit property that trimmed frames will never change type is sufficient.
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/414603002/80001
Message was sent while issue was closed.
Change committed as 287174 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
