[FFmpeg-devel] [PATCH 1/3] avcodec/h264, videotoolbox: pass SPS changes into the VT decoder

[Mark Thompson]

On 16/02/17 18:29, Aman Gupta wrote:
> From: Aman Gupta <aman at tmm1.net>
> 
> This fixes playback of h264 streams with SPS changes. One such sample
> is available at http://tmm1.s3.amazonaws.com/videotoolbox/spschange.ts.
> It switches mid-stream from level 4.0 to level 3.2.
> 
> Previously, playing this sample with the VT hwaccel on iOS would crash.
> After this patch, it plays back as expected.
> 
> On macOS however, feeding in new SPS into an existing decompression
> session does not always work, so this patch is only a partial fix.
> ---
>  libavcodec/h264dec.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
> index 41c0964..e521c52 100644
> --- a/libavcodec/h264dec.c
> +++ b/libavcodec/h264dec.c
> @@ -740,6 +740,13 @@ FF_ENABLE_DEPRECATION_WARNINGS
>              break;
>          case H264_NAL_SPS: {
>              GetBitContext tmp_gb = nal->gb;
> +            if (avctx->hwaccel && avctx->hwaccel->pix_fmt == AV_PIX_FMT_VIDEOTOOLBOX) {
> +                ret = avctx->hwaccel->decode_slice(avctx,
> +                                                   nal->data,
> +                                                   nal->size);
> +                if (ret < 0)
> +                    goto end;
> +            }
>              if (ff_h264_decode_seq_parameter_set(&tmp_gb, avctx, &h->ps, 0) >= 0)
>                  break;
>              av_log(h->avctx, AV_LOG_DEBUG,
> 

The SPS isn't a slice - calling decode_slice() on it is confusing.  I realise you are really using it as "upload blob" and that does basically make sense, but changes like this are moving it further away from being maintainable as a hwaccel.  (Compare how many DXVA- or VAAPI-specific code paths currently exist in the decoder outside the actual hwaccel functions.)  The previous patches doing the same thing with the PPS and fiddling something funny inside the reference picture lists are similar - in themselves not obviously unreasonable, but making the code trickier in ways which may make things more difficult later.

So, I think this needs a step back to consider whether this should actually be implemented as a hwaccel at all if changes like this are needed:

It looks like you are now supplying it all video-significant packets - what would need to be implemented to do this without being inside hwaccel infrastructure?  (As a standalone full-bitstream decoder, but possibly calling in to the existing H.264 code if that helps.)

On the other hand, if it is going to continue being a hwaccel, can we add make some changes to the internal hwaccel API to avoid having to make changes like this in the generic decoder?  (Also thinking of other codecs - you're changing H.264 here, are other codecs going to run into similar issues, particularly H.265 which is structurally very similar in these aspects?)