Have FFmpeg Decoders handle 0 byte buffers instead of FFmpeg

Have FFmpeg Decoders handle 0 byte buffers instead of FFmpeg

Starting in M60 avcodec_decode_video2() fails if an empty buffer is
passed in. So don't have FFmpegAudioDecoder or FFmpegVideoDecoder call
FFmpeg routines if there is no data to process.

BUG=718142, 663438
TEST=affected test still passes

Review-Url: https://codereview.chromium.org/2885643002
Cr-Commit-Position: refs/heads/master@{#474080}
Committed: https://chromium.googlesource.com/chromium/src/+/3287cb2391a584ab3b10c4a1099290fb419cd797

[jrummell, 2017-05-15 23:16:28]

jrummell@chromium.org changed reviewers:
+ wolenetz@chromium.org, xhwang@chromium.org

[jrummell, 2017-05-15 23:16:29]

PTAL. Preemptive fix for FFmpeg merge.

[xhwang, 2017-05-15 23:35:18]

Thanks for the fix. Shall we do the same for FFmpegAudioDecoder?

https://codereview.chromium.org/2885643002/diff/1/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/2885643002/diff/1/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.cc:353: // empty buffer provided. If that is
the case signal that more data
"If that is the case" seems to be confusing since we know what will happen?

https://codereview.chromium.org/2885643002/diff/1/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.cc:354: // is required to produce more
frames.
Could you please refer to crbug.com/663438 for context of 0-byte buffers

[jrummell, 2017-05-16 21:54:03]

Updated.

> Shall we do the same for FFmpegAudioDecoder?
FFmpegAudioDecoder doesn't have the same problem. If avcodec_decode_audio4()
fails then it simply logs an error and then returns true, which is what this
change does for FFmpegVideoDecoder (except no logging). So empty audio buffers
don't fail decoding of the audio stream.

https://codereview.chromium.org/2885643002/diff/1/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/2885643002/diff/1/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.cc:353: // empty buffer provided. If that is
the case signal that more data
On 2017/05/15 23:35:17, xhwang wrote:
> "If that is the case" seems to be confusing since we know what will happen?

Done.

https://codereview.chromium.org/2885643002/diff/1/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder.cc:354: // is required to produce more
frames.
On 2017/05/15 23:35:17, xhwang wrote:
> Could you please refer to crbug.com/663438 for context of 0-byte buffers

Done.

[xhwang, 2017-05-16 23:05:21]

On 2017/05/16 21:54:03, jrummell wrote:
> Updated.
> 
> > Shall we do the same for FFmpegAudioDecoder?
> FFmpegAudioDecoder doesn't have the same problem. If avcodec_decode_audio4()
> fails then it simply logs an error and then returns true, which is what this
> change does for FFmpegVideoDecoder (except no logging). So empty audio buffers
> don't fail decoding of the audio stream.

Right, but we'll still generate an error message in media-internals even though
it's not an error:

https://cs.chromium.org/chromium/src/media/filters/ffmpeg_audio_decoder.cc?rc...

I suggest we explicitly handle this using the same way in both decoders,
otherwise nobody would be able to understand this trickiness.

[jrummell, 2017-05-17 18:56:28]

Description was changed from

==========
Have FFmpegVideoDecoder handle 0 byte buffers instead of FFmpeg

Starting in M60 avcodec_decode_video2() fails if an empty buffer is
passed in. So don't have FFmpegVideoDecoder call it without data.

BUG=718142
TEST=affected test still passes
==========

to

==========
Have FFmpeg Decoders handle 0 byte buffers instead of FFmpeg

Starting in M60 avcodec_decode_video2() fails if an empty buffer is
passed in. So don't have FFmpegAudioDecoder or FFmpegVideoDecoder call
FFmpeg routines if there is no data to process.

BUG=718142
TEST=affected test still passes
==========

[jrummell, 2017-05-17 18:57:35]

Updated to include FFmpegAudioDecoder. Don't know for sure whether
avcodec_decode_audio4() complains or not, but having both decoders avoid this
case makes sense.

[jrummell, 2017-05-17 18:57:52]

Description was changed from

==========
Have FFmpeg Decoders handle 0 byte buffers instead of FFmpeg

Starting in M60 avcodec_decode_video2() fails if an empty buffer is
passed in. So don't have FFmpegAudioDecoder or FFmpegVideoDecoder call
FFmpeg routines if there is no data to process.

BUG=718142
TEST=affected test still passes
==========

to

==========
Have FFmpeg Decoders handle 0 byte buffers instead of FFmpeg

Starting in M60 avcodec_decode_video2() fails if an empty buffer is
passed in. So don't have FFmpegAudioDecoder or FFmpegVideoDecoder call
FFmpeg routines if there is no data to process.

BUG=718142,663438
TEST=affected test still passes
==========

[xhwang, 2017-05-17 20:57:49]

Thanks! LGTM

[jrummell, 2017-05-23 20:22:28]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2017-05-23 20:23:17]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 22:11:31]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495570948791660,
"parent_rev": "190125cd098bf1a71b7186b4278f6104aa0dbe6c", "commit_rev":
"3287cb2391a584ab3b10c4a1099290fb419cd797"}

[commit-bot: I haz the power, 2017-05-23 22:11:49]

Description was changed from

==========
Have FFmpeg Decoders handle 0 byte buffers instead of FFmpeg

Starting in M60 avcodec_decode_video2() fails if an empty buffer is
passed in. So don't have FFmpegAudioDecoder or FFmpegVideoDecoder call
FFmpeg routines if there is no data to process.

BUG=718142,663438
TEST=affected test still passes
==========

to

==========
Have FFmpeg Decoders handle 0 byte buffers instead of FFmpeg

Starting in M60 avcodec_decode_video2() fails if an empty buffer is
passed in. So don't have FFmpegAudioDecoder or FFmpegVideoDecoder call
FFmpeg routines if there is no data to process.

BUG=718142,663438
TEST=affected test still passes

Review-Url: https://codereview.chromium.org/2885643002
Cr-Commit-Position: refs/heads/master@{#474080}
Committed:
https://chromium.googlesource.com/chromium/src/+/3287cb2391a584ab3b10c4a10992...
==========

[commit-bot: I haz the power, 2017-05-23 22:11:50]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/3287cb2391a584ab3b10c4a10992...