[FFmpeg-devel] [PATCH] amfenc: Add support for pict_type field

[Mark Thompson]

On 15/01/2019 22:05, michael.dirks at xaymar.com wrote:
> From: Michael Fabian 'Xaymar' Dirks <info at xaymar.com>
> 
> Adds support for the pict_type field in AVFrame to amf_h264 and amf_h265 simultaneously. This field is needed in cases where the application wishes to override the frame type with another one, such as forcefully inserting a key frame for chapter markers or similar.
> 
> Additionally this abuses AV_PICTURE_TYPE_S for marking Skip frames, a special type of frame in AVC, SVC and HEVC which is a flag for the decoder to repeat the last frame.

Overloading the meaning of a flag like this is probably confusing.

Can you explain what this "skip frame" actually does in the encoder?  The concept does not exist in H.264 or H.265, as far as I know.

>  Changelog           |  1 +
>  libavcodec/amfenc.c | 46 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/Changelog b/Changelog
> index 1cd53916cb..4fc48ba210 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -15,6 +15,7 @@ version <next>:
>  - hymt decoder
>  - anlmdn filter
>  - maskfun filter
> +- AMF now supports AVFrame->pict_type override
>  
>  
>  version 4.1:
> diff --git a/libavcodec/amfenc.c b/libavcodec/amfenc.c
> index 384d8efc92..3be9ff9ee2 100644
> --- a/libavcodec/amfenc.c
> +++ b/libavcodec/amfenc.c
> @@ -680,6 +680,52 @@ int ff_amf_send_frame(AVCodecContext *avctx, const AVFrame *frame)
>              break;
>          }
>  
> +        // Override Picture Type for Frame
> +        if (avctx->codec->id == AV_CODEC_ID_H264) {
> +            switch (frame->pict_type) {
> +            case AV_PICTURE_TYPE_I:
> +                AMF_ASSIGN_PROPERTY_INT64(res, surface, AMF_VIDEO_ENCODER_FORCE_PICTURE_TYPE, AMF_VIDEO_ENCODER_PICTURE_TYPE_I);

Consider whether you need to set IDR here rather than I, and maybe add a comment explaining the choice?  The normal use of this is to indicate that an IRAP should be generated, not just a single intra picture.  (Compare libx264, which has an additional flag controlling whether the forced picture is IDR or I, but I believe it is still always an IRAP there.)

> +                break;
> +            case AV_PICTURE_TYPE_P:
> +                AMF_ASSIGN_PROPERTY_INT64(res, surface, AMF_VIDEO_ENCODER_FORCE_PICTURE_TYPE, AMF_VIDEO_ENCODER_PICTURE_TYPE_P);
> +                break;
> +            case AV_PICTURE_TYPE_B:
> +                AMF_ASSIGN_PROPERTY_INT64(res, surface, AMF_VIDEO_ENCODER_FORCE_PICTURE_TYPE, AMF_VIDEO_ENCODER_PICTURE_TYPE_B);
> +                break;
> +            case AV_PICTURE_TYPE_S:
> +                AMF_ASSIGN_PROPERTY_INT64(res, surface, AMF_VIDEO_ENCODER_FORCE_PICTURE_TYPE, AMF_VIDEO_ENCODER_PICTURE_TYPE_SKIP);
> +                break;
> +            default:
> +                AMF_ASSIGN_PROPERTY_INT64(res, surface, AMF_VIDEO_ENCODER_FORCE_PICTURE_TYPE, AMF_VIDEO_ENCODER_PICTURE_TYPE_NONE);
> +                break;
> +            }
> +            // Keyframe overrides previous assignment.
> +            if (frame->key_frame) {

This flag shouldn't be read by encoders.  (I believe it is set by the decoder and never cleared, so with this test you will have surprising behaviour where the output stream gets key frames everywhere that the input stream had them, in addition to those dictated by its own GOP structure.)

> +                AMF_ASSIGN_PROPERTY_INT64(res, surface, AMF_VIDEO_ENCODER_FORCE_PICTURE_TYPE, AMF_VIDEO_ENCODER_PICTURE_TYPE_IDR);
> +            }
> +        } else if (avctx->codec->id == AV_CODEC_ID_HEVC) {
> +            switch (frame->pict_type) {
> +            case AV_PICTURE_TYPE_I:
> +                AMF_ASSIGN_PROPERTY_INT64(res, surface, AMF_VIDEO_ENCODER_HEVC_FORCE_PICTURE_TYPE, AMF_VIDEO_ENCODER_HEVC_PICTURE_TYPE_I);
> +                break;
> +            case AV_PICTURE_TYPE_P:
> +                AMF_ASSIGN_PROPERTY_INT64(res, surface, AMF_VIDEO_ENCODER_HEVC_FORCE_PICTURE_TYPE, AMF_VIDEO_ENCODER_HEVC_PICTURE_TYPE_P);
> +                break;
> +            case AV_PICTURE_TYPE_B:
> +                av_log(ctx, AV_LOG_WARNING, "Ignoring B-Frame, unsupported by AMD AMF H.265 Encoder.");
> +                break;
> +            case AV_PICTURE_TYPE_S:
> +                AMF_ASSIGN_PROPERTY_INT64(res, surface, AMF_VIDEO_ENCODER_HEVC_FORCE_PICTURE_TYPE, AMF_VIDEO_ENCODER_HEVC_PICTURE_TYPE_SKIP);
> +                break;
> +            default:
> +                AMF_ASSIGN_PROPERTY_INT64(res, surface, AMF_VIDEO_ENCODER_HEVC_FORCE_PICTURE_TYPE, AMF_VIDEO_ENCODER_HEVC_PICTURE_TYPE_NONE);
> +                break;
> +            }
> +            // Keyframe overrides previous assignment.
> +            if (frame->key_frame) {
> +                AMF_ASSIGN_PROPERTY_INT64(res, surface, AMF_VIDEO_ENCODER_HEVC_FORCE_PICTURE_TYPE, AMF_VIDEO_ENCODER_HEVC_PICTURE_TYPE_IDR);
> +            }
> +        }
>  
>          // submit surface
>          res = ctx->encoder->pVtbl->SubmitInput(ctx->encoder, (AMFData*)surface);
> 

- Mark