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

[Mark Thompson]

On 04/02/2019 14:41, Michael Dirks wrote:
> On 04.02.2019 11:05, Mark Thompson wrote:
>> 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.
> 
> I believe this has to do with the pic_struct flag which has a value of 7 for frame doubling and 8 for frame tripling, see page 345 (PDF page 367) Table D-1 Interpretation of pic_struct in Rec. ITU-T H.264 (04/2017). However I do not work for AMD (and their encoder is closed source and a piece of hardware), so I can't confirm that this is actually the behavior, nor do I have any tools that can read and show all H.264 headers. An alternative would be that AMDs encoder is instead choosing to emit a frame that has maximum compression and references the previous frame for all data, thus causing a copy of it.

You can see this with the trace_headers bitstream filter.  (Try "-c:v h264_amf -options... -bsf:v trace_headers -f null -", or it can be used with streamcopy to examine an existing file.)

>>> +            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.)
>>> +            // 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.)
> I went by the documentation in the individual header files, which does not actually claim key_frame to be a decoder only field (libavutil/frame.h):
> 
>> /**
>>     * 1 -> keyframe, 0-> not
>>     */
>>    int key_frame;
> 
> Therefore this patch uses the field exactly as it is documented in the frame.h file, and also treats AV_PICTURE_TYPE_I as a request for an Intra Picture instead of an IDR Picture.

Well yes, but that's not actually going to work - try it with a set of JPEGs as input and you'll observe the problem.  (Note that no other encoder uses this field on input, though some do use it internally.)

- Mark