[FFmpeg-devel] A question regarding dangerous call inside libavformat\utils.c::has_decode_delay_been_guessed()

[Michael Niedermayer]

On Mon, Jul 25, 2016 at 01:55:18PM +0300, Ivan Uskov wrote:
> Hello Michael,
> 
> Sunday, July 24, 2016, 11:25:29 PM, you wrote:
> 
> MN> On Sun, Jul 24, 2016 at 11:18:38PM +0300, Ivan Uskov wrote:
> >> Hello Michael,
> >> 
> >> Sunday, July 24, 2016, 11:11:32 PM, you wrote:
> >> 
> >> MN> On Sun, Jul 24, 2016 at 09:55:21PM +0300, Ivan Uskov wrote:
> >> >> Hello All,
> >> >> 
> >> >> I have discovered the following issue:
> >> >> Latest builds of ffmpeg crashes into the h264.c when *hardware* qsv-based h264 decoder uses.
> >> >> The crash does appear inside the
> >> >> 
> >> >> int avpriv_h264_has_num_reorder_frames(AVCodecContext *avctx)
> >> >> {
> >> >>     H264Context *h = avctx->priv_data;
> >> >>     return h && h->ps.sps ? h->ps.sps->num_reorder_frames : 0;
> >> >> }
> >> >> 
> >> >> It is obvious, that casting to H264Context cannot be used for qsv decoder
> >> >> because there is QSVH2645Context. Similar issue will appear for CUVID
> >> >> decoder case (CuvidContext uses), Android MediaCodec H.264 decoder
> >> >> (MediaCodecH264DecContext uses), possible another cases existing.
> >> >> 
> >> >> The caller function is
> >> >> 
> >> >> static int has_decode_delay_been_guessed(AVStream *st)
> >> >> {
> >> >>     if (st->codecpar->codec_id != AV_CODEC_ID_H264) return 1;
> >> >>     if (!st->info) // if we have left find_stream_info then nb_decoded_frames won't increase anymore for stream copy
> >> >>         return 1;
> >> >> #if CONFIG_H264_DECODER
> >> >>     if (st->internal->avctx->has_b_frames &&
> >> >>        avpriv_h264_has_num_reorder_frames(st->internal->avctx) == st->internal->avctx->has_b_frames)
> >> >>         return 1;
> >> >> #endif
> >> >>     if (st->internal->avctx->has_b_frames<3)
> >> >>         return st->nb_decoded_frames >= 7;
> >> >>     else if (st->internal->avctx->has_b_frames<4)
> >> >>         return st->nb_decoded_frames >= 18;
> >> >>     else
> >> >>         return st->nb_decoded_frames >= 20;
> >> >> }
> >> >> ...which called by update_initial_timestamps()
> >> >> 
> >> >> Have anybody the idea how it can be correctly fixed?
> >> >> Looks like has_decode_delay_been_guessed() should be corrected.
> >> 
> >> MN> this code is not new, this is in git since 4 years
> >> MN> commit bafa1c7f383d6c1c0f3d207395fe6a574092b7ac
> >> MN> Date:   Mon Jul 2 23:16:59 2012 +020
> >> 
> >> MN> why does it cause a problem now ?
> >> 
> >> MN> why does libavformat use hw h264 decoding ?
> >> I do not know that is reason why I'm asking.
> >> I just run ffmpeg like
> >> ffmpeg -c:v h264_qsv -i ./Ducks.Take.Off.720p.QHD.CRF24.x264-CtrlHD.mkv -vcodec h264_qsv -y ./result.mp4
> >> ..and it crashes inside avpriv_h264_has_num_reorder_frames(AVCodecContext *avctx)
> >> 
> >> When I replace
> >> #if CONFIG_H264_DECODER
> >> to
> >> #if 0
> >> ...all working fine.
> >> 
> >> I can not say when exactly the issue appeared, about several weeks ago all
> >> worked correct. But current build is broken at this place.
> 
> MN> can you use git bisect to figure out when this issue appeared?
> The problem commit is
> Sun Jun 12 13:24:27 2016 +0200| [1534ef87c74cc66a117bf61c467641c2129bc964] | committer: Clément Bœsch
> 
> There are lot changes but this modification made the issue visible:
> ==========
> diff --git a/libavcodec/h264.c b/libavcodec/h264.c
> index c011527..0de6d91 100644
> --- a/libavcodec/h264.c
> +++ b/libavcodec/h264.c
>  <at>  <at>  -60,7 +60,7  <at>  <at>  const uint16_t ff_h264_mb_sizes[4] = { 256, 384, 512, 768 };
>  int avpriv_h264_has_num_reorder_frames(AVCodecContext *avctx)
>  {
>      H264Context *h = avctx->priv_data;
> -    return h ? h->sps.num_reorder_frames : 0;
> +    return h && h->ps.sps ? h->ps.sps->num_reorder_frames : 0;
>  }
> ==========
> I.e. _before_it_worked_wrong_too_ but silently. After the H264Context was
> modified and new construction h->ps.sps-> was added it has become critical.

hmm ok

so possible solutions are
A. make find_decoder() in libavformat/utils.c never return a hardware
   decoder, its a recipe for problems with races of thread unsafe
   drivers anyway
B. Improve the h264 AVParser in libavcodec so it can find the
   num_reorder_frames as well as the decoder and use that instead of
   avpriv_h264_has_num_reorder_frames()
C. disable avpriv_h264_has_num_reorder_frames() when the decoder isnt
   the sw decoder

C will cause errors for some usecases

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct answer.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160725/c91e1dfd/attachment.sig>