| Message ID | 1476924348-55856-1-git-send-email-ffmpeg@tmm1.net |
|---|---|
| State | New |
| Headers | show |
> 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
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 > >
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 >> >>
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.
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;
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(-)