Allow ffmpeg to process individual NALU rather than require frames.

Allow ffmpeg to process individual NALU rather than require frames.

The new video-decoding PPAPI accepts NALU's for H264 streams.

BUG=281689
TEST=Example in the pending CL for the new PPAPI

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275601

[igorc, 2014-05-30 17:44:32]

Note: I wasn't sure how to test this properly, so I'm open for suggestion. So
far I've used it with the in-progress video-decoding CL.

[DaleCurtis, 2014-05-30 17:49:06]

You'll need to run some numbers on software decoding for high res content if you
want to land this.  Especially for codecs where hardware decoding isn't
available.  vp8 and h264 on OSX specifically.

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

https://codereview.chromium.org/301243008/diff/1/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:360: codec_context_->flags2 |=
CODEC_FLAG2_CHUNKS;
This will disable frame threading for all clients which I don't think we want to
do since it will likely hurt performance significantly.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...

[DaleCurtis, 2014-05-30 17:51:48]

Hmm, I just realized we don't have the mediabench tool anymore.  The easiest way
to test would be to build ffmpeg_unittests and time decoding for longish high
res content.

[igorc, 2014-05-30 18:42:26]

On 2014/05/30 17:51:48, DaleCurtis wrote:
> Hmm, I just realized we don't have the mediabench tool anymore.  The easiest
way
> to test would be to build ffmpeg_unittests and time decoding for longish high
> res content.

What general approach do you think is the most useful?

I could see at least 3 possible options:
1. Test performance of the current state of CL and try to submit as is
    + Fewer options, less extra code
    - Affects more than what's actually has to be affected, needs performance
testing across platforms
2. Add a new option to VideoDecoderConfig, set it for PPAPI only
    + Precise control, still optimal for what can provide frames
    - An extra option, questions about testing
3. Reconstruct frames from NALU's at PPAPI or App level
    + Fewer options, still optimal on ffmpeg side
    - Extra complexity which looks unnecessary on the client side

I'm leaning towards #2, but it does have its negative sides.

[DaleCurtis, 2014-05-30 18:46:07]

You could dovetail this with the existing low delay flag since that already
disables frame threading.

[scherkus (not reviewing), 2014-05-30 20:57:37]

On 2014/05/30 18:46:07, DaleCurtis wrote:
> You could dovetail this with the existing low delay flag since that already
> disables frame threading.

I don't think we should conflate the two.

igorc: can you point us at calling code? it might be easier to expose a
setter/constructor parameter if you're using this class outside of
media::Pipeline

[igorc, 2014-05-30 21:04:25]

On 2014/05/30 20:57:37, scherkus wrote:
> On 2014/05/30 18:46:07, DaleCurtis wrote:
> > You could dovetail this with the existing low delay flag since that already
> > disables frame threading.
> 
> I don't think we should conflate the two.
> 
> igorc: can you point us at calling code? it might be easier to expose a
> setter/constructor parameter if you're using this class outside of
> media::Pipeline

This is all parts of Bill's CL that was not yet sent for review. I think it's
this code:

+  media::VideoDecoderConfig config(
+      codec,
+      profile,
+      media::VideoFrame::YV12,
+      gfx::Size(32, 24),  // Small sizes that won't fail.
+      gfx::Rect(32, 24),
+      gfx::Size(32, 24),
+      NULL /* extra_data */,
+      0 /* extra_data_size */,
+      false /* decryption */);
+
+  decoder_.reset(new media::FFmpegVideoDecoder(media_message_loop_));
+
+  media_message_loop_->PostTask(
+      FROM_HERE,
+      base::Bind(
+          &SoftwareDecoder::DoInitialize, base::Unretained(this), config));

+void SoftwareDecoder::DoInitialize(media::VideoDecoderConfig config) {
+  DCHECK(decoder_);
+  decoder_->Initialize(
+      config,
+      true /* low_delay */,
+      base::Bind(&SoftwareDecoder::OnPipelineStatus, base::Unretained(this)));
+}

[igorc, 2014-05-30 21:08:25]

On 2014/05/30 21:04:25, igorc wrote:
> On 2014/05/30 20:57:37, scherkus wrote:
> > On 2014/05/30 18:46:07, DaleCurtis wrote:
> > > You could dovetail this with the existing low delay flag since that
already
> > > disables frame threading.
> > 
> > I don't think we should conflate the two.
> > 
> > igorc: can you point us at calling code? it might be easier to expose a
> > setter/constructor parameter if you're using this class outside of
> > media::Pipeline
> 
> This is all parts of Bill's CL that was not yet sent for review. I think it's
> this code:
> 
> +  media::VideoDecoderConfig config(
> +      codec,
> +      profile,
> +      media::VideoFrame::YV12,
> +      gfx::Size(32, 24),  // Small sizes that won't fail.
> +      gfx::Rect(32, 24),
> +      gfx::Size(32, 24),
> +      NULL /* extra_data */,
> +      0 /* extra_data_size */,
> +      false /* decryption */);
> +
> +  decoder_.reset(new media::FFmpegVideoDecoder(media_message_loop_));
> +
> +  media_message_loop_->PostTask(
> +      FROM_HERE,
> +      base::Bind(
> +          &SoftwareDecoder::DoInitialize, base::Unretained(this), config));
> 
> +void SoftwareDecoder::DoInitialize(media::VideoDecoderConfig config) {
> +  DCHECK(decoder_);
> +  decoder_->Initialize(
> +      config,
> +      true /* low_delay */,
> +      base::Bind(&SoftwareDecoder::OnPipelineStatus,
base::Unretained(this)));
> +}

That code currently lives in
content/renderer/pepper/pepper_video_decoder_host.cc

[scherkus (not reviewing), 2014-05-30 22:51:06]

I see. Do you expect to be using other VideoDecoder implementations? (e.g.,
GpuVideoDecoder, VpxVideoDecoder)

If so, then I could see this as a param/modification to VideoDecoder.

If not, then a ctor param/setter on FFmpegVideoDecoder would work (e.g.,
FFmpegVideoDecoder::set_decode_nalus(bool decode_nalus) { ...} )

[bbudge, 2014-05-30 22:56:56]

On 2014/05/30 22:51:06, scherkus wrote:
> I see. Do you expect to be using other VideoDecoder implementations? (e.g.,
> GpuVideoDecoder, VpxVideoDecoder)
> 
> If so, then I could see this as a param/modification to VideoDecoder.
> 
> If not, then a ctor param/setter on FFmpegVideoDecoder would work (e.g.,
> FFmpegVideoDecoder::set_decode_nalus(bool decode_nalus) { ...} )

I will likely want to add VpxVideoDecoder (to handle VP9 in software fallback
mode).

[scherkus (not reviewing), 2014-05-30 23:08:31]

On 2014/05/30 22:56:56, bbudge wrote:
> On 2014/05/30 22:51:06, scherkus wrote:
> > I see. Do you expect to be using other VideoDecoder implementations? (e.g.,
> > GpuVideoDecoder, VpxVideoDecoder)
> > 
> > If so, then I could see this as a param/modification to VideoDecoder.
> > 
> > If not, then a ctor param/setter on FFmpegVideoDecoder would work (e.g.,
> > FFmpegVideoDecoder::set_decode_nalus(bool decode_nalus) { ...} )
> 
> I will likely want to add VpxVideoDecoder (to handle VP9 in software fallback
> mode).

Hrm ... IIRC VP[89] don't use NAL units so this really does seem like an request
that's exclusive to FFmpeg's H.264 decoder, in which case a setter works for me.

We could also DCHECK() that decode_nalus_ is true iff the codec is H264

[igorc, 2014-06-03 20:01:37]

On 2014/05/30 23:08:31, scherkus wrote:
> On 2014/05/30 22:56:56, bbudge wrote:
> > On 2014/05/30 22:51:06, scherkus wrote:
> > > I see. Do you expect to be using other VideoDecoder implementations?
(e.g.,
> > > GpuVideoDecoder, VpxVideoDecoder)
> > > 
> > > If so, then I could see this as a param/modification to VideoDecoder.
> > > 
> > > If not, then a ctor param/setter on FFmpegVideoDecoder would work (e.g.,
> > > FFmpegVideoDecoder::set_decode_nalus(bool decode_nalus) { ...} )
> > 
> > I will likely want to add VpxVideoDecoder (to handle VP9 in software
fallback
> > mode).
> 
> Hrm ... IIRC VP[89] don't use NAL units so this really does seem like an
request
> that's exclusive to FFmpeg's H.264 decoder, in which case a setter works for
me.
> 
> We could also DCHECK() that decode_nalus_ is true iff the codec is H264

Added SetDecodeNalus() (that name seems to better match other method names in
that class, as opposed to set_decode_nalus).

I did not add H264 DCHECK since:
a) I did not see any examples of the codec checks in the same file
b) Setting that flag for VP8 does not sound like a problem and could be ignored,
making caller code simpler
c) We should probably remove this flag once Bill starts to reconstruct frames
from NALU's in his software video decoder
... but I have no strong opinion here.

[igorc, 2014-06-05 00:02:10]

PTAL

[scherkus (not reviewing), 2014-06-06 18:12:27]

lgtm w/ nit

https://codereview.chromium.org/301243008/diff/20001/media/filters/ffmpeg_vid...
File media/filters/ffmpeg_video_decoder.h (right):

https://codereview.chromium.org/301243008/diff/20001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.h:36: void SetDecodeNalus(bool decode_nalus)
{ decode_nalus_ = decode_nalus; }
nit: style guide recommends unix_hacker() style for simple inline setters

i.e., set_decode_nalus(...) { ... }

[igorc, 2014-06-06 22:58:22]

https://codereview.chromium.org/301243008/diff/20001/media/filters/ffmpeg_vid...
File media/filters/ffmpeg_video_decoder.h (right):

https://codereview.chromium.org/301243008/diff/20001/media/filters/ffmpeg_vid...
media/filters/ffmpeg_video_decoder.h:36: void SetDecodeNalus(bool decode_nalus)
{ decode_nalus_ = decode_nalus; }
On 2014/06/06 18:12:27, scherkus wrote:
> nit: style guide recommends unix_hacker() style for simple inline setters
> 
> i.e., set_decode_nalus(...) { ... }

Done.

[igorc, 2014-06-06 23:09:24]

The CQ bit was checked by igorc@chromium.org

[commit-bot: I haz the power, 2014-06-06 23:10:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/igorc@chromium.org/301243008/40001

[commit-bot: I haz the power, 2014-06-07 03:43:32]

Change committed as 275601