[FFmpeg-devel] [PATCH v4 17/21] vaapi_encode_h264: Support stereo 3D metadata

[Mark Thompson]

On 25/02/2020 14:10, Fu, Linjie wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>> Mark Thompson
>> Sent: Monday, February 24, 2020 07:41
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: [FFmpeg-devel] [PATCH v4 17/21] vaapi_encode_h264: Support
>> stereo 3D metadata
>>
>> Insert frame packing arrangement messages into the stream when input
>> frames have associated stereo 3D side-data.
>> ---
>>  doc/encoders.texi              |  3 +++
>>  libavcodec/vaapi_encode_h264.c | 25 ++++++++++++++++++++++++-
>>  2 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/doc/encoders.texi b/doc/encoders.texi
>> index e23b6b32fe..62b6902197 100644
>> --- a/doc/encoders.texi
>> +++ b/doc/encoders.texi
>> @@ -3065,6 +3065,9 @@ Include picture timing parameters
>> (@emph{buffering_period} and
>>  @emph{pic_timing} messages).
>>  @item recovery_point
>>  Include recovery points where appropriate (@emph{recovery_point}
>> messages).
>> + at item frame_packing
>> +Include stereo 3D metadata if the input frames have it
>> +(@emph{frame_packing_arrangement} messages).
>>  @end table
>>
>>  @end table
>> diff --git a/libavcodec/vaapi_encode_h264.c
>> b/libavcodec/vaapi_encode_h264.c
>> index f4965d8b09..58eae613c4 100644
>> --- a/libavcodec/vaapi_encode_h264.c
>> +++ b/libavcodec/vaapi_encode_h264.c
>> @@ -25,6 +25,7 @@
>>  #include "libavutil/common.h"
>>  #include "libavutil/internal.h"
>>  #include "libavutil/opt.h"
>> +#include "libavutil/stereo3d.h"
>>
>>  #include "avcodec.h"
>>  #include "cbs.h"
>> @@ -39,6 +40,7 @@ enum {
>>      SEI_TIMING         = 0x01,
>>      SEI_IDENTIFIER     = 0x02,
>>      SEI_RECOVERY_POINT = 0x04,
>> +    SEI_FRAME_PACKING  = 0x20,
>>  };
> 
> There is a jumping from 0x04 to 0x20, how about combining it with the enum in
> vaapi_encode_h265.c, and moving into vaapi_encode.h, hence SEI_FRAME_PACKING
> could also be used for H265 later?

Yeah, the point of including the jump was that they are disjoint parts of the same enum in the two files.

Moving it into the header would be reasonable, I'll do that (other codecs where SEI isn't a thing can see it, but they don't care so whatever).

Does anyone use stereo frame packing in H.265?  If that's not an entirely vestigial feature then I would just add it, because it's very easy to do.

>>  // Random (version 4) ISO 11578 UUID.
>> @@ -96,6 +98,7 @@ typedef struct VAAPIEncodeH264Context {
>>      H264RawSEIBufferingPeriod      sei_buffering_period;
>>      H264RawSEIPicTiming            sei_pic_timing;
>>      H264RawSEIRecoveryPoint        sei_recovery_point;
>> +    H264RawSEIFramePackingArrangement sei_frame_packing;
>>      H264RawSEIUserDataUnregistered sei_identifier;
>>      char                          *sei_identifier_string;
>>
>> @@ -251,6 +254,12 @@ static int
>> vaapi_encode_h264_write_extra_header(AVCodecContext *avctx,
>>              sei->payload[i].payload.recovery_point = priv->sei_recovery_point;
>>              ++i;
>>          }
>> +        if (priv->sei_needed & SEI_FRAME_PACKING) {
>> +            sei->payload[i].payload_type = H264_SEI_TYPE_FRAME_PACKING;
>> +            sei->payload[i].payload.frame_packing_arrangement =
>> +                priv->sei_frame_packing;
>> +            ++i;
>> +        }
>>
>>          sei->payload_count = i;
>>          av_assert0(sei->payload_count > 0);
>> @@ -700,6 +709,17 @@ static int
>> vaapi_encode_h264_init_picture_params(AVCodecContext *avctx,
>>          priv->sei_needed |= SEI_RECOVERY_POINT;
>>      }
>>
>> +    if (priv->sei & SEI_FRAME_PACKING) {
>> +        AVFrameSideData *sd = av_frame_get_side_data(pic->input_image,
>> +                                                     AV_FRAME_DATA_STEREO3D);
>> +        if (sd) {
>> +            ff_cbs_h264_fill_sei_frame_packing_arrangement(
>> +                &priv->sei_frame_packing, (const AVStereo3D*)sd->data);
>> +        }
>> +
>> +        priv->sei_needed |= SEI_FRAME_PACKING;
> 
> If got NULL sd from av_frame_get_side_data(),  would it be better to not adding
> SEI_FRAME_PACKING to  priv->sei_needed or taking further actions to write extra header?

Good point, it needs to be inside the brace.  (And I should check negative cases more carefully.)

Thanks,

- Mark