Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
251893002:
Support start trimming post-decoding. Use it with FFmpegDemuxer. (Closed)
|
Created:
6 years, 8 months ago by DaleCurtis Modified:
6 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport start trimming post-decoding. Use it with FFmpegDemuxer.
FFmpeg has packet side data indicating how many frames should be
trimmed after decoding, we should use it to improve playback of
mp3 and aac audio via <audio> tag.
Specifically:
- AudioBuffer:TrimRange(start,end) is now supported.
- DecoderBuffer:discard_padding() is now a pair of (front, back)
which indicates how much to trim off the front and rear of the
data corresponding to the encoded buffer.
- AudioDiscardHelper has been updated to implement this trimming.
- FFmpegDemuxer inserts FFmpeg's skip_samples into DecoderBuffer.
This change paves the way for MediaSource to use this feature to
implement gapless playback support.
BUG=360961
TEST=new unittests!
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268002
Patch Set 1 : Typo. #
Total comments: 2
Patch Set 2 : Rebase. #
Total comments: 27
Patch Set 3 : Rebase. Fix TrimRange. #
Total comments: 30
Patch Set 4 : Comments. #
Total comments: 7
Messages
Total messages: 36 (0 generated)
wolenetz: Part II for review after https://codereview.chromium.org/259453003/ lands. acolwell: FYI again.
First glance comment :) (My full review will pend the landing of the prereq CL https://codereview.chromium.org/259453003/). https://codereview.chromium.org/251893002/diff/20001/media/base/audio_buffer_... File media/base/audio_buffer_unittest.cc (right): https://codereview.chromium.org/251893002/diff/20001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:16: static void VerifyResult(float* channel_data, At first glance, this seems to be incorrectly (re)based. Which CL introduced VerifyResult(), if not this one?
https://codereview.chromium.org/251893002/diff/20001/media/base/audio_buffer_... File media/base/audio_buffer_unittest.cc (right): https://codereview.chromium.org/251893002/diff/20001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:16: static void VerifyResult(float* channel_data, On 2014/04/28 23:02:02, wolenetz wrote: > At first glance, this seems to be incorrectly (re)based. Which CL introduced > VerifyResult(), if not this one? It's always been there? https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_b...
The baseline is off for this now a bit, i'll rebase after CQ'ing the part I.
On 2014/04/28 23:11:58, DaleCurtis wrote: > https://codereview.chromium.org/251893002/diff/20001/media/base/audio_buffer_... > File media/base/audio_buffer_unittest.cc (right): > > https://codereview.chromium.org/251893002/diff/20001/media/base/audio_buffer_... > media/base/audio_buffer_unittest.cc:16: static void VerifyResult(float* > channel_data, > On 2014/04/28 23:02:02, wolenetz wrote: > > At first glance, this seems to be incorrectly (re)based. Which CL introduced > > VerifyResult(), if not this one? > > It's always been there? > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_b... Silly me. My mistake for looking at the wrong file.
Rebased on the final version of part I.
First round of comments: I've only reviewed audio_buffer.{cc,h} so far.
https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer.cc
File media/base/audio_buffer.cc (right):
https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer....
media/base/audio_buffer.cc:284: const int frames_to_copy = adjusted_frame_count_
- frames_to_trim;
This is wrong for all cases where start > 0:
example:
trim_start_: 0
adjusted_frame_count_: 32
start: 15
end: 20
frames_to_copy should be 12 (== 32-20), not 27 (== 32-5).
Copying way too much is bad :) : (security, page fault, etc issues could occur).
Furthermore, I'm not convinced this method correctly understands any prior
TrimStart() which could have left trim_start_>0: What is the assumption of the
index-base of start and end?
Please fix and add test cases, too, to verify TrimStart() followed by
TrimRange() at least behave in sequence, possibly adding TrimEnd().
https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer....
media/base/audio_buffer.cc:285: switch (sample_format_) {
If frames_to_copy is 0, skip the shifting and just trim it all (rather than
memcpy(...,...,0) ?
https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer.h
File media/base/audio_buffer.h (right):
https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer....
media/base/audio_buffer.h:91: // and ending at |end|; i.e. exclusive. Even if
start is zero, timestamp() is
nit: only end is exclusive, right? Perhaps rephrase to something like: remove
frames [|start|, |end|) ?
On 2014/04/29 01:18:29, wolenetz wrote:
> First round of comments: I've only reviewed audio_buffer.{cc,h} so far.
>
>
https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer.cc
> File media/base/audio_buffer.cc (right):
>
>
https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer....
> media/base/audio_buffer.cc:284: const int frames_to_copy =
adjusted_frame_count_
> - frames_to_trim;
> This is wrong for all cases where start > 0:
> example:
> trim_start_: 0
> adjusted_frame_count_: 32
> start: 15
> end: 20
> frames_to_copy should be 12 (== 32-20), not 27 (== 32-5).
> Copying way too much is bad :) : (security, page fault, etc issues could
occur).
>
> Furthermore, I'm not convinced this method correctly understands any prior
> TrimStart() which could have left trim_start_>0: What is the assumption of the
> index-base of start and end?
>
> Please fix and add test cases, too, to verify TrimStart() followed by
> TrimRange() at least behave in sequence, possibly adding TrimEnd().
>
>
https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer....
> media/base/audio_buffer.cc:285: switch (sample_format_) {
> If frames_to_copy is 0, skip the shifting and just trim it all (rather than
> memcpy(...,...,0) ?
>
> https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer.h
> File media/base/audio_buffer.h (right):
>
>
https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer....
> media/base/audio_buffer.h:91: // and ending at |end|; i.e. exclusive. Even if
> start is zero, timestamp() is
> nit: only end is exclusive, right? Perhaps rephrase to something like: remove
> frames [|start|, |end|) ?
Also, I'm beginning to suspect I don't understand expected TrimRange behavior.
Note that memcpy is bad if dest and src ranges overlap: "To avoid overflows, the
size of the arrays pointed by both the destination and source parameters, shall
be at least num bytes, and should not overlap (for overlapping memory blocks,
memmove is a safer approach)."
https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer.... media/base/audio_buffer.cc:284: const int frames_to_copy = adjusted_frame_count_ - frames_to_trim; On 2014/04/29 01:18:30, wolenetz wrote: > This is wrong for all cases where start > 0: > example: > trim_start_: 0 > adjusted_frame_count_: 32 > start: 15 > end: 20 > frames_to_copy should be 12 (== 32-20), not 27 (== 32-5). > Copying way too much is bad :) : (security, page fault, etc issues could occur). > > Furthermore, I'm not convinced this method correctly understands any prior > TrimStart() which could have left trim_start_>0: What is the assumption of the > index-base of start and end? > > Please fix and add test cases, too, to verify TrimStart() followed by > TrimRange() at least behave in sequence, possibly adding TrimEnd(). Yeah this is all wrong, I'm surprised it works at all! I promise it passed all the tests :) The concept will be the same once I fix the implementation, so the rest of CL is good for further review.
Review still in progress. A few more nits/comments: https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer.... media/base/audio_buffer.cc:290: memcpy(channel_data_[ch] + start * bytes_per_channel, Use memmove, not memcpy due to potentially overlapping ranges (per msg#8) ? https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer.... media/base/audio_buffer.cc:301: memcpy(channel_data_[0] + start * frame_size, ditto: memmove? https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... File media/base/audio_buffer_unittest.cc (right): https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:24: start += increment; nits: If MakeAudioBuffer was subject to error using this methodology (see previous CL https://codereview.chromium.org/259453003/), why not use ASSERT_EQ(channel_data[i], start + increment * i)? Also change to const all of this method's params? https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:366: const base::TimeDelta duration = base::TimeDelta::FromSeconds(frames); Do you really intend duration to be 100 seconds, even though 100 frames at 44100 is only ~0.002268 seconds? If so, please document this maybe in the |duration| variable name like "overestimated_duration". https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:369: MakeAudioBuffer<float>(kSampleFormatPlanarF32, TrimRange does fancy mem ops depending on planar vs interleaved. Please test both. https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:389: VerifyResult(bus->channel(0), trim_start, 0, step); TrimRange's fancy mem ops could use additional verification: VerifyResult of all 4 channels please. https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:395: EXPECT_EQ(buffer->duration(), base::TimeDelta::FromSeconds(50)); I don't understand why an AudioBuffer could be given a duration that is out-of-whack versus its frame count and sample rate. Is there a use case that justifies this allowance? https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:399: VerifyResult(bus->channel(0), trim_start, 0, step); ditto: verify all channels https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:403: VerifyResult(bus->channel(0), trim_start, trim_start + trim_length, step); ditto https://codereview.chromium.org/251893002/diff/40001/media/filters/ffmpeg_dem... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/251893002/diff/40001/media/filters/ffmpeg_dem... media/filters/ffmpeg_demuxer.cc:208: buffer->set_discard_padding(std::make_pair( nit: as discussed, possibly restrict ffmpeg skip_samples usage to just the first buffer?
https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... File media/base/audio_buffer_unittest.cc (right): https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:395: EXPECT_EQ(buffer->duration(), base::TimeDelta::FromSeconds(50)); On 2014/04/29 19:18:08, wolenetz wrote: > I don't understand why an AudioBuffer could be given a duration that is > out-of-whack versus its frame count and sample rate. Is there a use case that > justifies this allowance? Generally the duration will only be off by a microsecond or so; it was required before AudioBuffer's had sample rates. Now that they do, we can probably rework this a bit. I suspect removing the ability to set_duration() would break a lot of tests which perform the equivalent of something like ASSERT_EQ(timestamp_helper_duration, buffer_duration). I'll see about a separate CL which removes set_duration().
Making acolwell a reviewer since it's unclear how long wolenetz@ might be out. PTAL. TrimRange() is now fixed and this is rebased on top of https://codereview.chromium.org/261533002/ https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer.... media/base/audio_buffer.cc:285: switch (sample_format_) { On 2014/04/29 01:18:30, wolenetz wrote: > If frames_to_copy is 0, skip the shifting and just trim it all (rather than > memcpy(...,...,0) ? Done. https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer.... media/base/audio_buffer.cc:290: memcpy(channel_data_[ch] + start * bytes_per_channel, On 2014/04/29 19:18:08, wolenetz wrote: > Use memmove, not memcpy due to potentially overlapping ranges (per msg#8) ? Done. https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer.... media/base/audio_buffer.cc:301: memcpy(channel_data_[0] + start * frame_size, On 2014/04/29 19:18:08, wolenetz wrote: > ditto: memmove? Done. https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer.h File media/base/audio_buffer.h (right): https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer.... media/base/audio_buffer.h:91: // and ending at |end|; i.e. exclusive. Even if start is zero, timestamp() is On 2014/04/29 01:18:30, wolenetz wrote: > nit: only end is exclusive, right? Perhaps rephrase to something like: remove > frames [|start|, |end|) ? Done. https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... File media/base/audio_buffer_unittest.cc (right): https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:24: start += increment; On 2014/04/29 19:18:08, wolenetz wrote: > nits: If MakeAudioBuffer was subject to error using this methodology (see > previous CL https://codereview.chromium.org/259453003/), why not use > ASSERT_EQ(channel_data[i], start + increment * i)? > Also change to const all of this method's params? Fixed here: https://codereview.chromium.org/261533002/ https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:366: const base::TimeDelta duration = base::TimeDelta::FromSeconds(frames); On 2014/04/29 19:18:08, wolenetz wrote: > Do you really intend duration to be 100 seconds, even though 100 frames at 44100 > is only ~0.002268 seconds? If so, please document this maybe in the |duration| > variable name like "overestimated_duration". Fixed here https://codereview.chromium.org/261533002/ and in new tests. https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:369: MakeAudioBuffer<float>(kSampleFormatPlanarF32, On 2014/04/29 19:18:08, wolenetz wrote: > TrimRange does fancy mem ops depending on planar vs interleaved. Please test > both. Done. https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:389: VerifyResult(bus->channel(0), trim_start, 0, step); On 2014/04/29 19:18:08, wolenetz wrote: > TrimRange's fancy mem ops could use additional verification: VerifyResult of all > 4 channels please. Done. https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:395: EXPECT_EQ(buffer->duration(), base::TimeDelta::FromSeconds(50)); On 2014/04/29 20:46:35, DaleCurtis wrote: > On 2014/04/29 19:18:08, wolenetz wrote: > > I don't understand why an AudioBuffer could be given a duration that is > > out-of-whack versus its frame count and sample rate. Is there a use case that > > justifies this allowance? > > Generally the duration will only be off by a microsecond or so; it was required > before AudioBuffer's had sample rates. Now that they do, we can probably rework > this a bit. > > I suspect removing the ability to set_duration() would break a lot of tests > which perform the equivalent of something like > ASSERT_EQ(timestamp_helper_duration, buffer_duration). > > I'll see about a separate CL which removes set_duration(). Fixed here: https://codereview.chromium.org/261533002/ https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:399: VerifyResult(bus->channel(0), trim_start, 0, step); On 2014/04/29 19:18:08, wolenetz wrote: > ditto: verify all channels Done. https://codereview.chromium.org/251893002/diff/40001/media/base/audio_buffer_... media/base/audio_buffer_unittest.cc:403: VerifyResult(bus->channel(0), trim_start, trim_start + trim_length, step); On 2014/04/29 19:18:08, wolenetz wrote: > ditto Done. https://codereview.chromium.org/251893002/diff/40001/media/filters/ffmpeg_dem... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/251893002/diff/40001/media/filters/ffmpeg_dem... media/filters/ffmpeg_demuxer.cc:208: buffer->set_discard_padding(std::make_pair( On 2014/04/29 19:18:08, wolenetz wrote: > nit: as discussed, possibly restrict ffmpeg skip_samples usage to just the first > buffer? Done.
Here is my initial pass. Most of the comments are questions to help me understand. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/251893002/diff/100001/media/base/audio_buffer... media/base/audio_buffer.cc:259: CHECK_LE(frames_to_trim, adjusted_frame_count_);; nit: remove extra ; https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... File media/base/audio_discard_helper.cc (right): https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.cc:86: delayed_discard_.swap(current_encoded_buffer); Why do you need the whole buffer? It looks like you only actually use the discard_padding() everywhere below. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.cc:112: TimeDeltaToFrames(current_encoded_buffer->discard_padding().first); Why isn't discard padding in frames? You appear to convert it from frames to time in the demuxer and here you are converting it back to frames. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.cc:116: size_t discard_start = codec_delay_; Isn't this baking in an assumption that discard_padding() is only set for the first buffer decoded? https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.cc:130: CHECK_LT(discard_start, decoded_frames); Does this hold for Opus? I was under the impression that the codec delay could span several buffers. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.cc:156: CHECK(!codec_delay_); Please add a TODO and file a bug for this since I believe this essentially blocks this feature for Opus. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... File media/base/audio_discard_helper.h (right): https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.h:22: // |sample_rate| is the sample rate of decode data which will be handed into nit: s/decode/decoded/ ? https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... File media/base/audio_discard_helper_unittest.cc (right): https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper_unittest.cc:296: std::make_pair(kDuration / 4, kDuration / 4)); How about using different fractions for the start, end, and initial discard? I feel like that would make it easier to see that one of these values isn't simply being copied. How about something like 1/16, 1/8, and 1/4? I believe that should result in 9/16 left right? https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper_unittest.cc:311: const int kCodecDelay = kSampleRate / 100 / 2; How about making this 80ms, but keeping the 10ms decode buffers so it can simulate a possible situation in Opus? https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper_unittest.cc:362: std::make_pair(kDuration / 4, kDuration / 4)); Ditto w/ respect to using different fractions here as well. https://codereview.chromium.org/251893002/diff/100001/media/filters/opus_audi... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/251893002/diff/100001/media/filters/opus_audi... media/filters/opus_audio_decoder.cc:422: new AudioDiscardHelper(config_.samples_per_second(), 0)); Is this correct? Shouldn't the config.coding_delay() get passed in here?
https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... File media/base/audio_discard_helper.cc (right): https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.cc:86: delayed_discard_.swap(current_encoded_buffer); On 2014/05/01 01:08:34, acolwell wrote: > Why do you need the whole buffer? It looks like you only actually use the > discard_padding() everywhere below. Good point, I'll only store that in the next patch set. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.cc:112: TimeDeltaToFrames(current_encoded_buffer->discard_padding().first); On 2014/05/01 01:08:34, acolwell wrote: > Why isn't discard padding in frames? You appear to convert it from frames to > time in the demuxer and here you are converting it back to frames. It's in TimeDelta since the MSE case will be based the delta in append window, you can see a similar approach in my gapless CL here: https://codereview.chromium.org/227713008/diff/1/media/filters/legacy_frame_p... If we use frames, I don't know how the frame processor will figure out the conversion. I'm happy to change this all to frames if you see a way to make that work. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.cc:116: size_t discard_start = codec_delay_; On 2014/05/01 01:08:34, acolwell wrote: > Isn't this baking in an assumption that discard_padding() is only set for the > first buffer decoded? No, this code should work for start discard padding on any buffer, hence the calculation against codec_delay. Only in the FFmpeg case where codec delay is munged with skip samples do we have to avoid later buffers having discard padding. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.cc:130: CHECK_LT(discard_start, decoded_frames); On 2014/05/01 01:08:34, acolwell wrote: > Does this hold for Opus? I was under the impression that the codec delay could > span several buffers. We may need to discuss this part more, since I'm a little fuzzy on OPUS, this does seem to work in testing though. My understanding is that codec delay is encoded into the OPUS packets, such that the packets representing the first AudioDecoderConfig::codec_delay() of decoded frames are all junk. My evidence for this is that the opus_multistream_decode_float() call is a 1-1 mapping. Once the end of stream buffer is received there's nothing left. See the comment here: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/opus... Because of this |codec_delay_| in AudioDiscardHelper is actually set to zero for OPUS. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.cc:156: CHECK(!codec_delay_); On 2014/05/01 01:08:34, acolwell wrote: > Please add a TODO and file a bug for this since I believe this essentially > blocks this feature for Opus. As far as I can tell this all works fine. See my comment above. https://codereview.chromium.org/251893002/diff/100001/media/filters/opus_audi... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/251893002/diff/100001/media/filters/opus_audi... media/filters/opus_audio_decoder.cc:422: new AudioDiscardHelper(config_.samples_per_second(), 0)); On 2014/05/01 01:08:34, acolwell wrote: > Is this correct? Shouldn't the config.coding_delay() get passed in here? Not as far as I can tell, see my previous comment :)
I retested with the mega trimming test clip from http://crbug.com/178309 and the OPUS discarding seems correct. I'll try to create some MSE test cases from that and verify further. https://codereview.chromium.org/251893002/diff/100001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/251893002/diff/100001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:201: const int kSkipEndSamplesOffset = 4; Note to self: This is wrong now, it should be in uint32 bytes, so just 1.
Ok. In general I think I'm ok with the changes. I think most of my confusion is related to variable naming. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... File media/base/audio_discard_helper.cc (right): https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.cc:112: TimeDeltaToFrames(current_encoded_buffer->discard_padding().first); On 2014/05/01 01:26:32, DaleCurtis wrote: > On 2014/05/01 01:08:34, acolwell wrote: > > Why isn't discard padding in frames? You appear to convert it from frames to > > time in the demuxer and here you are converting it back to frames. > > It's in TimeDelta since the MSE case will be based the delta in append window, > you can see a similar approach in my gapless CL here: > > https://codereview.chromium.org/227713008/diff/1/media/filters/legacy_frame_p... > > If we use frames, I don't know how the frame processor will figure out the > conversion. I'm happy to change this all to frames if you see a way to make that > work. Ok. I see. I don't have a good alternte proposal right now. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.cc:117: if (codec_delay_ > 0) { I think this should be renamed too because this name caused me to assume that this path would be taken for Opus, but based on your comments and our offline discussion, it isn't. https://codereview.chromium.org/251893002/diff/100001/media/filters/opus_audi... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/251893002/diff/100001/media/filters/opus_audi... media/filters/opus_audio_decoder.cc:422: new AudioDiscardHelper(config_.samples_per_second(), 0)); On 2014/05/01 01:26:32, DaleCurtis wrote: > On 2014/05/01 01:08:34, acolwell wrote: > > Is this correct? Shouldn't the config.coding_delay() get passed in here? > > Not as far as I can tell, see my previous comment :) Ok. This feels real counter intuitive to me. Perhaps the parameter just needs a different name because it seems strange that Opus has a coding delay, but we are essentially telling the AudioDiscardHelper that it doesn't. It feels like this parameter means something different than coding delay. It just happens to equal the coding delay in some cases.
https://codereview.chromium.org/251893002/diff/100001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/251893002/diff/100001/media/base/audio_buffer... media/base/audio_buffer.cc:259: CHECK_LE(frames_to_trim, adjusted_frame_count_);; On 2014/05/01 01:08:34, acolwell wrote: > nit: remove extra ; Done. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... File media/base/audio_discard_helper.cc (right): https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.cc:86: delayed_discard_.swap(current_encoded_buffer); On 2014/05/01 01:26:32, DaleCurtis wrote: > On 2014/05/01 01:08:34, acolwell wrote: > > Why do you need the whole buffer? It looks like you only actually use the > > discard_padding() everywhere below. > > Good point, I'll only store that in the next patch set. Done. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.cc:117: if (codec_delay_ > 0) { On 2014/05/01 17:44:26, acolwell wrote: > I think this should be renamed too because this name caused me to assume that > this path would be taken for Opus, but based on your comments and our offline > discussion, it isn't. Done. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... File media/base/audio_discard_helper.h (right): https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper.h:22: // |sample_rate| is the sample rate of decode data which will be handed into On 2014/05/01 01:08:34, acolwell wrote: > nit: s/decode/decoded/ ? Done. I've also renamed codec_delay to decoder_delay and added some docs about exactly what that means. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... File media/base/audio_discard_helper_unittest.cc (right): https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper_unittest.cc:296: std::make_pair(kDuration / 4, kDuration / 4)); On 2014/05/01 01:08:34, acolwell wrote: > How about using different fractions for the start, end, and initial discard? I > feel like that would make it easier to see that one of these values isn't simply > being copied. How about something like 1/16, 1/8, and 1/4? I believe that should > result in 9/16 left right? Done. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper_unittest.cc:311: const int kCodecDelay = kSampleRate / 100 / 2; On 2014/05/01 01:08:34, acolwell wrote: > How about making this 80ms, but keeping the 10ms decode buffers so it can > simulate a possible situation in Opus? As discussed this doesn't apply for opus and for complexity reasons isn't allowed in the code. See the various DCHECKs around ensuring the discard padding is within the current buffer and that if a buffer is entirely discarded it contains no discard padding. https://codereview.chromium.org/251893002/diff/100001/media/base/audio_discar... media/base/audio_discard_helper_unittest.cc:362: std::make_pair(kDuration / 4, kDuration / 4)); On 2014/05/01 01:08:34, acolwell wrote: > Ditto w/ respect to using different fractions here as well. Done. https://codereview.chromium.org/251893002/diff/100001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/251893002/diff/100001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:201: const int kSkipEndSamplesOffset = 4; On 2014/05/01 01:44:24, DaleCurtis wrote: > Note to self: This is wrong now, it should be in uint32 bytes, so just 1. Done. https://codereview.chromium.org/251893002/diff/100001/media/filters/opus_audi... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/251893002/diff/100001/media/filters/opus_audi... media/filters/opus_audio_decoder.cc:422: new AudioDiscardHelper(config_.samples_per_second(), 0)); On 2014/05/01 17:44:26, acolwell wrote: > On 2014/05/01 01:26:32, DaleCurtis wrote: > > On 2014/05/01 01:08:34, acolwell wrote: > > > Is this correct? Shouldn't the config.coding_delay() get passed in here? > > > > Not as far as I can tell, see my previous comment :) > Ok. This feels real counter intuitive to me. Perhaps the parameter just needs a > different name because it seems strange that Opus has a coding delay, but we are > essentially telling the AudioDiscardHelper that it doesn't. It feels like this > parameter means something different than coding delay. It just happens to equal > the coding delay in some cases. As mentioned this is has been renamed and documented.
lgtm
The CQ bit was checked by dalecurtis@chromium.org
CQ'ing! wolenetz@ confirmed he was okay with go ahead for now, any issues found will be addressed in follow up CLs.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/251893002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium
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/251893002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium
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/251893002/120001
The CQ bit was unchecked by dalecurtis@chromium.org
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/251893002/120001
Message was sent while issue was closed.
Change committed as 268002
Message was sent while issue was closed.
I'm sorry for my belated CR on this. https://codereview.chromium.org/251893002/diff/120001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/251893002/diff/120001/media/base/audio_buffer... media/base/audio_buffer.cc:275: const int frames_to_copy = adjusted_frame_count_ - end; nit: remove extra whitespace https://codereview.chromium.org/251893002/diff/120001/media/base/audio_buffer... File media/base/audio_buffer_unittest.cc (right): https://codereview.chromium.org/251893002/diff/120001/media/base/audio_buffer... media/base/audio_buffer_unittest.cc:69: bus.get(), trim_start, buffer->frame_count() - trim_start, 0, 1); I'm not sure this is correct and how this is not failing. It looks to me like the original (non-trimmed) buffer values are what VerifyBusWithOffset() is expecting, not the relatively higher values resulting from the trim+move of higher values earlier into the buffer+bus. In other words, where is trim_length communicated to (or inferred by) VerifyBusWithOffset()? Similar applies below. Update: Offline chat with dalecurtis@ looks like this comment has resulted in: https://codereview.chromium.org/261333002/ https://codereview.chromium.org/251893002/diff/120001/media/base/audio_discar... File media/base/audio_discard_helper.cc (right): https://codereview.chromium.org/251893002/diff/120001/media/base/audio_discar... media/base/audio_discard_helper.cc:99: // For simplicity disallow cases where a buffer with discard padding is nit: how common is this? should we track it in release builds (change to CHECKs?) https://codereview.chromium.org/251893002/diff/120001/media/formats/webm/webm... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/251893002/diff/120001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.cc:421: base::TimeDelta::FromMicroseconds(discard_padding / 1000))); Matroska allows for negative DiscardPadding to indicate start discard, and positive for end discard. Do any of our WebM-parsed formats possibly include negative DiscardPadding (start discard)? If so, account for it here. If not, add DLOG(ERROR... or CHECK(... ?
Message was sent while issue was closed.
https://codereview.chromium.org/251893002/diff/120001/media/base/audio_buffer.cc File media/base/audio_buffer.cc (right): https://codereview.chromium.org/251893002/diff/120001/media/base/audio_buffer... media/base/audio_buffer.cc:275: const int frames_to_copy = adjusted_frame_count_ - end; On 2014/05/05 19:01:23, wolenetz wrote: > nit: remove extra whitespace Never mind. My mistake for not seeing the '_' :)
Message was sent while issue was closed.
https://codereview.chromium.org/251893002/diff/120001/media/base/audio_discar... File media/base/audio_discard_helper.cc (right): https://codereview.chromium.org/251893002/diff/120001/media/base/audio_discar... media/base/audio_discard_helper.cc:99: // For simplicity disallow cases where a buffer with discard padding is On 2014/05/05 19:01:23, wolenetz wrote: > nit: how common is this? should we track it in release builds (change to > CHECKs?) It shouldn't be possible right now, so the DCHECK() should be sufficient. https://codereview.chromium.org/251893002/diff/120001/media/formats/webm/webm... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/251893002/diff/120001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.cc:421: base::TimeDelta::FromMicroseconds(discard_padding / 1000))); On 2014/05/05 19:01:23, wolenetz wrote: > Matroska allows for negative DiscardPadding to indicate start discard, and > positive for end discard. > > Do any of our WebM-parsed formats possibly include negative DiscardPadding > (start discard)? If so, account for it here. If not, add DLOG(ERROR... or > CHECK(... ? I didn't know that. If WebM supports that, probably this code should be setting start padding to -discard_padding then. There'a a DCHECK in the AudioDiscardHelper against negative values. acolwell: WDYT? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
