Update FFmpegAudioDecoder to support multi-frame packets.

Update FFmpegAudioDecoder to support multi-frame packets.

Per the FFmpeg docs, avcodec_decode_audio4 should be repeatedly
called until all data from a given packet is exhausted.

Ran into this issue while running the FFmpeg regression tests after
acolwell added a DCHECK to ensure we were always fully exhausting
the packet after decode.

BUG=141020
TEST=ffmpeg_regression_tests
TBR=jam


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154220

[DaleCurtis, 2012-08-28 01:18:18]

acolwell: Please review.
rbultje: Sanity check for FFmpeg API usage.

[DaleCurtis, 2012-08-28 01:56:44]

On 2012/08/28 01:18:18, DaleCurtis wrote:
> acolwell: Please review.
> rbultje: Sanity check for FFmpeg API usage.

Also, rbultje: can you clarify if we need to do anything for video? My reading
of the docs says no and ffplay doesn't seem to expect multiple frames per packet
for video.

[acolwell GONE FROM CHROMIUM, 2012-08-28 14:40:49]

Looks pretty good. Just a few comments

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.cc:172: if (!queued_audio_.empty()) {
flip condition, early return, & drop the else

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.cc:175:
queued_audio_.erase(queued_audio_.begin());
nit: use pop_front()

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.cc:271: break;
It looks like you should be able to unconditionally break here and
ReadFromDemuxerStream() will still get called.

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.cc:302: continue;
This should break & not continue decoding.

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.cc:337: output = new DataBuffer(0);
You should set packet.size = 0 here since we don't want to generate multiple end
of stream buffers.

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.cc:347: statistics_cb_.Run(statistics);
I think this needs to be moved into the bottom of the loop.

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.cc:359:
queued_audio_.erase(queued_audio_.begin());
use pop_front()

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
File media/filters/ffmpeg_audio_decoder.h (right):

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.h:99: std::vector<QueuedAudioBuffer>
queued_audio_;
use a std::list here so popping from the front doesn't require copying all the
elements to shift them.

[rbultje1, 2012-08-28 16:14:38]

On 2012/08/28 01:56:44, DaleCurtis wrote:
> On 2012/08/28 01:18:18, DaleCurtis wrote:
> > acolwell: Please review.
> > rbultje: Sanity check for FFmpeg API usage.
> 
> Also, rbultje: can you clarify if we need to do anything for video? My reading
> of the docs says no and ffplay doesn't seem to expect multiple frames per
packet
> for video.

No, you don't need anything special for video; the demuxer (assuming you use
av_read_frame()) will do video-frame parsing automatically. If you use
av_read_packet(), you can do manual frame-parsing using AVCodecParser or so.

For audio, we don't, since frame-level parsing adds a lot of overhead at no
gain, just think of voice or pcm codecs, where a "frame" really is just a few
bytes.

[rbultje1, 2012-08-28 16:16:22]

On 2012/08/28 01:18:18, DaleCurtis wrote:
> acolwell: Please review.
> rbultje: Sanity check for FFmpeg API usage.

API usage LGTM.

[DaleCurtis, 2012-08-28 20:19:16]

+crogers FYI since this fixes two WebAudio issues:
- Containers with multiple streams failing decode.
- Multi-frame packets.

PTAL. This now requires https://chromiumcodereview.appspot.com/10871051/ to land
first.

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.cc:172: if (!queued_audio_.empty()) {
On 2012/08/28 14:40:49, acolwell wrote:
> flip condition, early return, & drop the else

Done.

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.cc:175:
queued_audio_.erase(queued_audio_.begin());
On 2012/08/28 14:40:49, acolwell wrote:
> nit: use pop_front()

Done.

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.cc:271: break;
On 2012/08/28 14:40:49, acolwell wrote:
> It looks like you should be able to unconditionally break here and
> ReadFromDemuxerStream() will still get called.

Done.

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.cc:302: continue;
On 2012/08/28 14:40:49, acolwell wrote:
> This should break & not continue decoding.

Done.

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.cc:337: output = new DataBuffer(0);
On 2012/08/28 14:40:49, acolwell wrote:
> You should set packet.size = 0 here since we don't want to generate multiple
end
> of stream buffers.

Should already be zero here, changed to DCHECK.

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.cc:347: statistics_cb_.Run(statistics);
On 2012/08/28 14:40:49, acolwell wrote:
> I think this needs to be moved into the bottom of the loop.

Done.

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.cc:359:
queued_audio_.erase(queued_audio_.begin());
On 2012/08/28 14:40:49, acolwell wrote:
> use pop_front()

Done.

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
File media/filters/ffmpeg_audio_decoder.h (right):

https://chromiumcodereview.appspot.com/10869085/diff/6001/media/filters/ffmpe...
media/filters/ffmpeg_audio_decoder.h:99: std::vector<QueuedAudioBuffer>
queued_audio_;
On 2012/08/28 14:40:49, acolwell wrote:
> use a std::list here so popping from the front doesn't require copying all the
> elements to shift them.

Oh no, I'm in CS 101 all over again. /hangs head in shame.

https://chromiumcodereview.appspot.com/10869085/diff/17001/media/filters/audi...
File media/filters/audio_file_reader.cc (right):

https://chromiumcodereview.appspot.com/10869085/diff/17001/media/filters/audi...
media/filters/audio_file_reader.cc:135: // Skip packets from other streams.
Noticed this while testing, previously handing a container with multiple streams
to WebAudio would fail on the first packet not belonging to this decoder.

https://chromiumcodereview.appspot.com/10869085/diff/17001/media/filters/ffmp...
File media/filters/ffmpeg_audio_decoder_unittest.cc (right):

https://chromiumcodereview.appspot.com/10869085/diff/17001/media/filters/ffmp...
media/filters/ffmpeg_audio_decoder_unittest.cc:149: .Times(4);
Patch removes the statistics callback for end of stream buffers with size == 0.

[DaleCurtis, 2012-08-28 20:19:48]

+crogers for FYI for real this time.

[acolwell GONE FROM CHROMIUM, 2012-08-28 20:31:53]

LGTM

https://chromiumcodereview.appspot.com/10869085/diff/17001/media/filters/audi...
File media/filters/audio_file_reader.cc (right):

https://chromiumcodereview.appspot.com/10869085/diff/17001/media/filters/audi...
media/filters/audio_file_reader.cc:163: if (!frame_decoded)
Does there need to be a if (packet_temp.size == 0) break; here so that more
packets can be fetched?

[DaleCurtis, 2012-08-28 21:41:17]

https://chromiumcodereview.appspot.com/10869085/diff/17001/media/filters/audi...
File media/filters/audio_file_reader.cc (right):

https://chromiumcodereview.appspot.com/10869085/diff/17001/media/filters/audi...
media/filters/audio_file_reader.cc:163: if (!frame_decoded)
On 2012/08/28 20:31:53, acolwell wrote:
> Does there need to be a if (packet_temp.size == 0) break; here so that more
> packets can be fetched?

I'm not sure I follow; this continue affects the inner loop so it should go to
the while (packet_temp.size() > 0) check below?

[acolwell GONE FROM CHROMIUM, 2012-08-28 21:45:04]

https://chromiumcodereview.appspot.com/10869085/diff/17001/media/filters/audi...
File media/filters/audio_file_reader.cc (right):

https://chromiumcodereview.appspot.com/10869085/diff/17001/media/filters/audi...
media/filters/audio_file_reader.cc:163: if (!frame_decoded)
On 2012/08/28 21:41:17, DaleCurtis wrote:
> On 2012/08/28 20:31:53, acolwell wrote:
> > Does there need to be a if (packet_temp.size == 0) break; here so that more
> > packets can be fetched?
> 
> I'm not sure I follow; this continue affects the inner loop so it should go to
> the while (packet_temp.size() > 0) check below?
oh.. right..nevermind. I'm so used to 'continue' meaning go up to the top I
forgot about the while check below. :)

[commit-bot: I haz the power, 2012-08-30 15:57:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10869085/17001

[commit-bot: I haz the power, 2012-08-30 19:15:29]

Change committed as 154220