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

[Aman Gupta]

On Sun, Oct 23, 2016 at 11:20 PM, Aman Gupta <aman at tmm1.net> wrote:

>
>
> On Sunday, October 23, 2016, Richard Kern <kernrj at gmail.com> wrote:
>
>>
>> > On Oct 19, 2016, at 8:45 PM, Aman Gupta <ffmpeg at tmm1.net> wrote:
>> >
>> > From: Aman Gupta <aman at 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 at ffmpeg.org
>> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>>