[FFmpeg-devel] lavc/videotoolbox: fix avcc creation for h264 streams missing extradata

Submitted by Aman Gupta on Oct. 20, 2016, 12:45 a.m.

Details

Message ID 1476924348-55856-1-git-send-email-ffmpeg@tmm1.net
State New
Headers show

Commit Message

Aman Gupta Oct. 20, 2016, 12:45 a.m.
From: Aman Gupta <aman@tmm1.net>

ff_videotoolbox_avcc_extradata_create() was never being called if
avctx->extradata_size==0, even though the function does not need or use
the avctx->extradata.

This manifested itself only on h264 streams in specific containers, and
only on iOS. I guess the macOS version of VideoToolbox is more
forgiving, atleast on my specific combination of OS version and hardware.

I also added an error log message when VTDecompressionSessionCreate()
fails, to help the next poor soul who runs into a bug in this area of the
code. The native OSStatus error codes are much easier to google than
their AVERROR counterparts (especially in this case, with AVERROR_UNKNOWN).
---
 libavcodec/videotoolbox.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Richard Kern Oct. 24, 2016, 1:09 a.m.
> On Oct 19, 2016, at 8:45 PM, Aman Gupta <ffmpeg@tmm1.net> wrote:
> 
> From: Aman Gupta <aman@tmm1.net>
> 
> ff_videotoolbox_avcc_extradata_create() was never being called if
> avctx->extradata_size==0, even though the function does not need or use
> the avctx->extradata.

Could this be a bug in another part of the code? It seems like extradata should be set.

> 
> This manifested itself only on h264 streams in specific containers, and
> only on iOS. I guess the macOS version of VideoToolbox is more
> forgiving, atleast on my specific combination of OS version and hardware.

Which container has this issue?

> 
> I also added an error log message when VTDecompressionSessionCreate()
> fails, to help the next poor soul who runs into a bug in this area of the
> code. The native OSStatus error codes are much easier to google than
> their AVERROR counterparts (especially in this case, with AVERROR_UNKNOWN).
> ---
> libavcodec/videotoolbox.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
> index 1288aa5..b21eccb 100644
> --- a/libavcodec/videotoolbox.c
> +++ b/libavcodec/videotoolbox.c
> @@ -413,7 +413,7 @@ static CFDictionaryRef videotoolbox_decoder_config_create(CMVideoCodecType codec
>                          kVTVideoDecoderSpecification_RequireHardwareAcceleratedVideoDecoder,
>                          kCFBooleanTrue);
> 
> -    if (avctx->extradata_size) {
> +    if (avctx->extradata_size || codec_type == kCMVideoCodecType_H264) {

This is somewhat confusing. The extradata_size check is only needed for videotoolbox_esds_extradata_create(). It should check the sps and pps sizes are valid before creating the avcc atom.

>         CFMutableDictionaryRef avc_info;
>         CFDataRef data = NULL;
> 
> @@ -572,13 +572,16 @@ static int videotoolbox_default_init(AVCodecContext *avctx)
>     if (buf_attr)
>         CFRelease(buf_attr);
> 
> +    if (status != 0)
> +        av_log(avctx, AV_LOG_ERROR, "Error creating videotoolbox decompression session: %d\n", status);
> +
>     switch (status) {
>     case kVTVideoDecoderNotAvailableNowErr:
>     case kVTVideoDecoderUnsupportedDataFormatErr:
>         return AVERROR(ENOSYS);
>     case kVTVideoDecoderMalfunctionErr:
>         return AVERROR(EINVAL);
> -    case kVTVideoDecoderBadDataErr :
> +    case kVTVideoDecoderBadDataErr:
>         return AVERROR_INVALIDDATA;
>     case 0:
>         return 0;
> -- 
> 2.8.1
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Aman Gupta Oct. 24, 2016, 6:20 a.m.
On Sunday, October 23, 2016, Richard Kern <kernrj@gmail.com> wrote:

>
> > On Oct 19, 2016, at 8:45 PM, Aman Gupta <ffmpeg@tmm1.net <javascript:;>>
> wrote:
> >
> > From: Aman Gupta <aman@tmm1.net <javascript:;>>
> >
> > ff_videotoolbox_avcc_extradata_create() was never being called if
> > avctx->extradata_size==0, even though the function does not need or use
> > the avctx->extradata.
>
> Could this be a bug in another part of the code? It seems like extradata
> should be set.


Yes it definitely could be. I can verify if extradata is present on macOS
where the same sample and ffmpeg version worked.


>
> >
> > This manifested itself only on h264 streams in specific containers, and
> > only on iOS. I guess the macOS version of VideoToolbox is more
> > forgiving, atleast on my specific combination of OS version and hardware.
>
> Which container has this issue?


I saw it with an mpegts file, but only on iOS.


>
> >
> > I also added an error log message when VTDecompressionSessionCreate()
> > fails, to help the next poor soul who runs into a bug in this area of the
> > code. The native OSStatus error codes are much easier to google than
> > their AVERROR counterparts (especially in this case, with
> AVERROR_UNKNOWN).
> > ---
> > libavcodec/videotoolbox.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
> > index 1288aa5..b21eccb 100644
> > --- a/libavcodec/videotoolbox.c
> > +++ b/libavcodec/videotoolbox.c
> > @@ -413,7 +413,7 @@ static CFDictionaryRef videotoolbox_decoder_config_create(CMVideoCodecType
> codec
> >                          kVTVideoDecoderSpecification_
> RequireHardwareAcceleratedVideoDecoder,
> >                          kCFBooleanTrue);
> >
> > -    if (avctx->extradata_size) {
> > +    if (avctx->extradata_size || codec_type == kCMVideoCodecType_H264) {
>
> This is somewhat confusing. The extradata_size check is only needed for
> videotoolbox_esds_extradata_create(). It should check the sps and pps
> sizes are valid before creating the avcc atom.


I can remove this outer if statement altogether, and add a check either
around or inside the esds_create if that makes more sense.


>
> >         CFMutableDictionaryRef avc_info;
> >         CFDataRef data = NULL;
> >
> > @@ -572,13 +572,16 @@ static int videotoolbox_default_init(AVCodecContext
> *avctx)
> >     if (buf_attr)
> >         CFRelease(buf_attr);
> >
> > +    if (status != 0)
> > +        av_log(avctx, AV_LOG_ERROR, "Error creating videotoolbox
> decompression session: %d\n", status);
> > +
> >     switch (status) {
> >     case kVTVideoDecoderNotAvailableNowErr:
> >     case kVTVideoDecoderUnsupportedDataFormatErr:
> >         return AVERROR(ENOSYS);
> >     case kVTVideoDecoderMalfunctionErr:
> >         return AVERROR(EINVAL);
> > -    case kVTVideoDecoderBadDataErr :
> > +    case kVTVideoDecoderBadDataErr:
> >         return AVERROR_INVALIDDATA;
> >     case 0:
> >         return 0;
> > --
> > 2.8.1
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org <javascript:;>
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
Aman Gupta Oct. 24, 2016, 7:18 p.m.
On Sun, Oct 23, 2016 at 11:20 PM, Aman Gupta <aman@tmm1.net> wrote:

>
>
> On Sunday, October 23, 2016, Richard Kern <kernrj@gmail.com> wrote:
>
>>
>> > On Oct 19, 2016, at 8:45 PM, Aman Gupta <ffmpeg@tmm1.net> wrote:
>> >
>> > From: Aman Gupta <aman@tmm1.net>
>> >
>> > ff_videotoolbox_avcc_extradata_create() was never being called if
>> > avctx->extradata_size==0, even though the function does not need or use
>> > the avctx->extradata.
>>
>> Could this be a bug in another part of the code? It seems like extradata
>> should be set.
>
>
> Yes it definitely could be. I can verify if extradata is present on macOS
> where the same sample and ffmpeg version worked.
>

Looks like missing extradata has nothing to do with the platform, but
rather only happens when you skip calling avformat_find_stream_info().

I am skipping the find_stream_info() call to reduce time spent probing and
begin playback more quickly, since the initial avformat_open_input() is
enough to discover available streams on mpegts containers.

That means this issue is not as widespread as I initially thought, but I
still think it's an optimization worth making.


>
>
>>
>> >
>> > This manifested itself only on h264 streams in specific containers, and
>> > only on iOS. I guess the macOS version of VideoToolbox is more
>> > forgiving, atleast on my specific combination of OS version and
>> hardware.
>>
>> Which container has this issue?
>
>
> I saw it with an mpegts file, but only on iOS.
>
>
>>
>> >
>> > I also added an error log message when VTDecompressionSessionCreate()
>> > fails, to help the next poor soul who runs into a bug in this area of
>> the
>> > code. The native OSStatus error codes are much easier to google than
>> > their AVERROR counterparts (especially in this case, with
>> AVERROR_UNKNOWN).
>> > ---
>> > libavcodec/videotoolbox.c | 7 +++++--
>> > 1 file changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
>> > index 1288aa5..b21eccb 100644
>> > --- a/libavcodec/videotoolbox.c
>> > +++ b/libavcodec/videotoolbox.c
>> > @@ -413,7 +413,7 @@ static CFDictionaryRef
>> videotoolbox_decoder_config_create(CMVideoCodecType codec
>> >                          kVTVideoDecoderSpecification_R
>> equireHardwareAcceleratedVideoDecoder,
>> >                          kCFBooleanTrue);
>> >
>> > -    if (avctx->extradata_size) {
>> > +    if (avctx->extradata_size || codec_type == kCMVideoCodecType_H264)
>> {
>>
>> This is somewhat confusing. The extradata_size check is only needed for
>> videotoolbox_esds_extradata_create(). It should check the sps and pps
>> sizes are valid before creating the avcc atom.
>
>
> I can remove this outer if statement altogether, and add a check either
> around or inside the esds_create if that makes more sense.
>

Let me know if you'd like me to submit the alternative version of this
patch.


>
>
>>
>> >         CFMutableDictionaryRef avc_info;
>> >         CFDataRef data = NULL;
>> >
>> > @@ -572,13 +572,16 @@ static int videotoolbox_default_init(AVCodecContext
>> *avctx)
>> >     if (buf_attr)
>> >         CFRelease(buf_attr);
>> >
>> > +    if (status != 0)
>> > +        av_log(avctx, AV_LOG_ERROR, "Error creating videotoolbox
>> decompression session: %d\n", status);
>> > +
>> >     switch (status) {
>> >     case kVTVideoDecoderNotAvailableNowErr:
>> >     case kVTVideoDecoderUnsupportedDataFormatErr:
>> >         return AVERROR(ENOSYS);
>> >     case kVTVideoDecoderMalfunctionErr:
>> >         return AVERROR(EINVAL);
>> > -    case kVTVideoDecoderBadDataErr :
>> > +    case kVTVideoDecoderBadDataErr:
>> >         return AVERROR_INVALIDDATA;
>> >     case 0:
>> >         return 0;
>> > --
>> > 2.8.1
>> >
>> > _______________________________________________
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel@ffmpeg.org
>> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>>
wm4 Oct. 24, 2016, 7:25 p.m.
On Mon, 24 Oct 2016 12:18:07 -0700
Aman Gupta <ffmpeg@tmm1.net> wrote:

> On Sun, Oct 23, 2016 at 11:20 PM, Aman Gupta <aman@tmm1.net> wrote:
> 
> >
> >
> > On Sunday, October 23, 2016, Richard Kern <kernrj@gmail.com> wrote:
> >  
> >>  
> >> > On Oct 19, 2016, at 8:45 PM, Aman Gupta <ffmpeg@tmm1.net> wrote:
> >> >
> >> > From: Aman Gupta <aman@tmm1.net>
> >> >
> >> > ff_videotoolbox_avcc_extradata_create() was never being called if
> >> > avctx->extradata_size==0, even though the function does not need or use
> >> > the avctx->extradata.  
> >>
> >> Could this be a bug in another part of the code? It seems like extradata
> >> should be set.  
> >
> >
> > Yes it definitely could be. I can verify if extradata is present on macOS
> > where the same sample and ffmpeg version worked.
> >  
> 
> Looks like missing extradata has nothing to do with the platform, but
> rather only happens when you skip calling avformat_find_stream_info().
> 
> I am skipping the find_stream_info() call to reduce time spent probing and
> begin playback more quickly, since the initial avformat_open_input() is
> enough to discover available streams on mpegts containers.
> 
> That means this issue is not as widespread as I initially thought, but I
> still think it's an optimization worth making.

That makes sense. I think a decoder shouldn't really rely on this
pseudo extradata that libavformat constructs.

That this code is run only with extradata_size set makes no sense at
least in the h264 code and was an oversight.

Patch hide | download patch | download mbox

diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
index 1288aa5..b21eccb 100644
--- a/libavcodec/videotoolbox.c
+++ b/libavcodec/videotoolbox.c
@@ -413,7 +413,7 @@  static CFDictionaryRef videotoolbox_decoder_config_create(CMVideoCodecType codec
                          kVTVideoDecoderSpecification_RequireHardwareAcceleratedVideoDecoder,
                          kCFBooleanTrue);
 
-    if (avctx->extradata_size) {
+    if (avctx->extradata_size || codec_type == kCMVideoCodecType_H264) {
         CFMutableDictionaryRef avc_info;
         CFDataRef data = NULL;
 
@@ -572,13 +572,16 @@  static int videotoolbox_default_init(AVCodecContext *avctx)
     if (buf_attr)
         CFRelease(buf_attr);
 
+    if (status != 0)
+        av_log(avctx, AV_LOG_ERROR, "Error creating videotoolbox decompression session: %d\n", status);
+
     switch (status) {
     case kVTVideoDecoderNotAvailableNowErr:
     case kVTVideoDecoderUnsupportedDataFormatErr:
         return AVERROR(ENOSYS);
     case kVTVideoDecoderMalfunctionErr:
         return AVERROR(EINVAL);
-    case kVTVideoDecoderBadDataErr :
+    case kVTVideoDecoderBadDataErr:
         return AVERROR_INVALIDDATA;
     case 0:
         return 0;