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

Michael Dirks michael.dirks at xaymar.com
Mon Feb 4 16:41:40 EET 2019


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.

>> +            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.

Sincerely
Michael Fabian Dirks



More information about the ffmpeg-devel mailing list