diff mbox series

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

Message ID 20200223234124.17689-17-sw@jkqxz.net
State New
Headers show
Series [FFmpeg-devel,v4,01/21] cbs: Mention all codecs in unit type comment | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Mark Thompson Feb. 23, 2020, 11:41 p.m. UTC 
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(-)

Comments

Fu, Linjie Feb. 25, 2020, 2:10 p.m. UTC | #1 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Mark Thompson
> Sent: Monday, February 24, 2020 07:41
> To: ffmpeg-devel@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).
> +@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?

vaapi_encode_h265.c:
enum {
    SEI_MASTERING_DISPLAY       = 0x08,
    SEI_CONTENT_LIGHT_LEVEL     = 0x10,
};

>  // 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?

- Linjie
Mark Thompson Feb. 25, 2020, 11:19 p.m. UTC | #2 
On 25/02/2020 14:10, Fu, Linjie wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Mark Thompson
>> Sent: Monday, February 24, 2020 07:41
>> To: ffmpeg-devel@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).
>> +@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
diff mbox series

Patch

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).
+@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,
 };
 
 // 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;
+    }
+
     vpic->CurrPic = (VAPictureH264) {
         .picture_id          = pic->recon_surface,
         .frame_idx           = hpic->frame_num,
@@ -1271,7 +1291,7 @@  static const AVOption vaapi_encode_h264_options[] = {
 
     { "sei", "Set SEI to include",
       OFFSET(sei), AV_OPT_TYPE_FLAGS,
-      { .i64 = SEI_IDENTIFIER | SEI_TIMING | SEI_RECOVERY_POINT },
+      { .i64 = SEI_IDENTIFIER | SEI_TIMING | SEI_RECOVERY_POINT | SEI_FRAME_PACKING },
       0, INT_MAX, FLAGS, "sei" },
     { "identifier", "Include encoder version identifier",
       0, AV_OPT_TYPE_CONST, { .i64 = SEI_IDENTIFIER },
@@ -1282,6 +1302,9 @@  static const AVOption vaapi_encode_h264_options[] = {
     { "recovery_point", "Include recovery points where appropriate",
       0, AV_OPT_TYPE_CONST, { .i64 = SEI_RECOVERY_POINT },
       INT_MIN, INT_MAX, FLAGS, "sei" },
+    { "frame_packing", "Include frame packing arrangement for stereo 3D",
+      0, AV_OPT_TYPE_CONST, { .i64 = SEI_FRAME_PACKING },
+      INT_MIN, INT_MAX, FLAGS, "sei" },
 
     { "profile", "Set profile (profile_idc and constraint_set*_flag)",
       OFFSET(profile), AV_OPT_TYPE_INT,