[FFmpeg-devel] added sei side data

Added seperate side data field to allow adding per packet side data
message to support MISB 604 encoding
---
 libavcodec/avpacket.c          |   1 +
 libavcodec/h264_metadata_bsf.c | 116 +++++++++++++++++++--------------
 libavcodec/packet.h            |   5 ++
 3 files changed, 72 insertions(+), 50 deletions(-)

Thanks. I have the same need.

Brad

On Mon, Jun 01, 2020 at 02:23:23PM -0700, Daniel Loman wrote:
> Added seperate side data field to allow adding per packet side data
> message to support MISB 604 encoding
> ---
>  libavcodec/avpacket.c          |   1 +
>  libavcodec/h264_metadata_bsf.c | 116 +++++++++++++++++++--------------
>  libavcodec/packet.h            |   5 ++
>  3 files changed, 72 insertions(+), 50 deletions(-)
> 
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index 033f2d8f26..a530dc6779 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -395,6 +395,7 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type)
>      case AV_PKT_DATA_A53_CC:                     return "A53 Closed Captions";
>      case AV_PKT_DATA_ENCRYPTION_INIT_INFO:       return "Encryption initialization data";
>      case AV_PKT_DATA_ENCRYPTION_INFO:            return "Encryption info";
> +    case AV_PKT_DATA_SEI_USER:                   return "SEI unregistered data";
>      case AV_PKT_DATA_AFD:                        return "Active Format Description data";
>      case AV_PKT_DATA_PRFT:                       return "Producer Reference Time";
>      case AV_PKT_DATA_ICC_PROFILE:                return "ICC Profile";
> diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
> index 99017653d0..e90b82793b 100644
> --- a/libavcodec/h264_metadata_bsf.c
> +++ b/libavcodec/h264_metadata_bsf.c
> @@ -276,6 +276,65 @@ static int h264_metadata_update_sps(AVBSFContext *bsf,
>      return 0;
>  }
>  
> +static int write_sei_user_data(AVBSFContext *bsf, const uint8_t *data, int size)
> +{
> +    H264MetadataContext *ctx = bsf->priv_data;
> +    CodedBitstreamFragment *au = &ctx->access_unit;
> +    int err = 0, i, j;
> +
> +    H264RawSEIPayload payload = {
> +        .payload_type = H264_SEI_TYPE_USER_DATA_UNREGISTERED,
> +    };
> +    H264RawSEIUserDataUnregistered *udu =
> +        &payload.payload.user_data_unregistered;
> +
> +    for (i = j = 0; j < 32 && data[i]; i++) {
> +        int c, v;
> +        c = data[i];
> +        if (c == '-') {
> +            continue;
> +        } else if (av_isxdigit(c)) {
> +            c = av_tolower(c);
> +            v = (c <= '9' ? c - '0' : c - 'a' + 10);
> +        } else {
> +            goto invalid_user_data;
> +        }
> +        if (i & 1)
> +            udu->uuid_iso_iec_11578[j / 2] |= v;
> +        else
> +            udu->uuid_iso_iec_11578[j / 2] = v << 4;
> +        ++j;
> +    }
> +    if (j == 32 && data[i] == '+') {
> +        size_t len = size - i - 1;
> +
> +        udu->data_ref = av_buffer_alloc(len + 1);
> +        if (!udu->data_ref) {
> +            err = AVERROR(ENOMEM);
> +            return err;
> +        }
> +
> +        udu->data        = udu->data_ref->data;
> +        udu->data_length = len + 1;
> +        memcpy(udu->data, data + i + 1, len + 1);
> +
> +        err = ff_cbs_h264_add_sei_message(ctx->cbc, au, &payload);
> +        if (err < 0) {
> +            av_log(bsf, AV_LOG_ERROR, "Failed to add user data SEI "
> +                   "message to access unit.\n");
> +            return err;
> +        }
> +
> +    } else {
> +    invalid_user_data:
> +        av_log(bsf, AV_LOG_ERROR, "Invalid user data: "
> +               "must be \"UUID+string\".\n");
> +        err = AVERROR(EINVAL);
> +        return err;
> +    }
> +    return err;
> +}
> +
>  static int h264_metadata_update_side_data(AVBSFContext *bsf, AVPacket *pkt)
>  {
>      H264MetadataContext *ctx = bsf->priv_data;
> @@ -412,56 +471,7 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt)
>      // Only insert the SEI in access units containing SPSs, and also
>      // unconditionally in the first access unit we ever see.
>      if (ctx->sei_user_data && (has_sps || !ctx->done_first_au)) {
> -        H264RawSEIPayload payload = {
> -            .payload_type = H264_SEI_TYPE_USER_DATA_UNREGISTERED,
> -        };
> -        H264RawSEIUserDataUnregistered *udu =
> -            &payload.payload.user_data_unregistered;
> -
> -        for (i = j = 0; j < 32 && ctx->sei_user_data[i]; i++) {
> -            int c, v;
> -            c = ctx->sei_user_data[i];
> -            if (c == '-') {
> -                continue;
> -            } else if (av_isxdigit(c)) {
> -                c = av_tolower(c);
> -                v = (c <= '9' ? c - '0' : c - 'a' + 10);
> -            } else {
> -                goto invalid_user_data;
> -            }
> -            if (j & 1)
> -                udu->uuid_iso_iec_11578[j / 2] |= v;
> -            else
> -                udu->uuid_iso_iec_11578[j / 2] = v << 4;
> -            ++j;
> -        }
> -        if (j == 32 && ctx->sei_user_data[i] == '+') {
> -            size_t len = strlen(ctx->sei_user_data + i + 1);
> -
> -            udu->data_ref = av_buffer_alloc(len + 1);
> -            if (!udu->data_ref) {
> -                err = AVERROR(ENOMEM);
> -                goto fail;
> -            }
> -
> -            udu->data        = udu->data_ref->data;
> -            udu->data_length = len + 1;
> -            memcpy(udu->data, ctx->sei_user_data + i + 1, len + 1);
> -
> -            err = ff_cbs_h264_add_sei_message(ctx->cbc, au, &payload);
> -            if (err < 0) {
> -                av_log(bsf, AV_LOG_ERROR, "Failed to add user data SEI "
> -                       "message to access unit.\n");
> -                goto fail;
> -            }
> -
> -        } else {
> -        invalid_user_data:
> -            av_log(bsf, AV_LOG_ERROR, "Invalid user data: "
> -                   "must be \"UUID+string\".\n");
> -            err = AVERROR(EINVAL);
> -            goto fail;
> -        }
> +      write_sei_user_data(bsf, ctx->sei_user_data, strlen(ctx->sei_user_data));

1, it's bad indention.
2, you didn't process the return of write_sei_user_data.
3, it's better to split into another patch for this change.

>      }
>  
>      if (ctx->delete_filler) {
> @@ -604,6 +614,12 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt)
>          }
>      }
>  
> +    uint8_t *data;
> +    int size;

mixed declaration

> +    if (data = av_packet_get_side_data(pkt, AV_PKT_DATA_SEI_USER, &size)) {
> +        write_sei_user_data(bsf, data, size);
> +    }
> +
>      err = ff_cbs_write_packet(ctx->cbc, pkt, au);
>      if (err < 0) {
>          av_log(bsf, AV_LOG_ERROR, "Failed to write packet.\n");
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index 41485f4527..48e0ccbaf0 100644
> --- a/libavcodec/packet.h
> +++ b/libavcodec/packet.h
> @@ -282,6 +282,11 @@ enum AVPacketSideDataType {
>       */
>      AV_PKT_DATA_DOVI_CONF,
>  
> +    /**
> +     * This side data contains SEI unregistered Data.
> +     */
> +    AV_PKT_DATA_SEI_USER,
> +

it's public header

>      /**
>       * The number of side data types.
>       * This is not part of the public API/ABI in the sense that it may
> -- 
> 2.17.1
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

>> --git a/libavcodec/packet.h b/libavcodec/packet.h index 
>> 41485f4527..48e0ccbaf0 100644
>> --- a/libavcodec/packet.h
>> +++ b/libavcodec/packet.h
>> @@ -282,6 +282,11 @@ enum AVPacketSideDataType {
>>       */
>>      AV_PKT_DATA_DOVI_CONF,
>>  
>> +    /**
>> +     * This side data contains SEI unregistered Data.
>> +     */
>> +    AV_PKT_DATA_SEI_USER,
>> +
>
>it's public header

I don't understand this comment. What are you expecting as the change here?

Brad

Daniel Loman:
> Added seperate side data field to allow adding per packet side data
> message to support MISB 604 encoding
> ---
>  libavcodec/avpacket.c          |   1 +
>  libavcodec/h264_metadata_bsf.c | 116 +++++++++++++++++++--------------

The changes to h264_metadata should be in a separate commit after the
addition of the new packet side data type. Furthermore, the commit
adding the new packet side data type needs to update the version and add
a changelog entry.

>  libavcodec/packet.h            |   5 ++
>  3 files changed, 72 insertions(+), 50 deletions(-)
> 
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index 033f2d8f26..a530dc6779 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -395,6 +395,7 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type)
>      case AV_PKT_DATA_A53_CC:                     return "A53 Closed Captions";
>      case AV_PKT_DATA_ENCRYPTION_INIT_INFO:       return "Encryption initialization data";
>      case AV_PKT_DATA_ENCRYPTION_INFO:            return "Encryption info";
> +    case AV_PKT_DATA_SEI_USER:                   return "SEI unregistered data";
>      case AV_PKT_DATA_AFD:                        return "Active Format Description data";
>      case AV_PKT_DATA_PRFT:                       return "Producer Reference Time";
>      case AV_PKT_DATA_ICC_PROFILE:                return "ICC Profile";
> diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
> index 99017653d0..e90b82793b 100644
> --- a/libavcodec/h264_metadata_bsf.c
> +++ b/libavcodec/h264_metadata_bsf.c
> @@ -276,6 +276,65 @@ static int h264_metadata_update_sps(AVBSFContext *bsf,
>      return 0;
>  }
>  
> +static int write_sei_user_data(AVBSFContext *bsf, const uint8_t *data, int size)
> +{
> +    H264MetadataContext *ctx = bsf->priv_data;
> +    CodedBitstreamFragment *au = &ctx->access_unit;
> +    int err = 0, i, j;
> +
> +    H264RawSEIPayload payload = {
> +        .payload_type = H264_SEI_TYPE_USER_DATA_UNREGISTERED,
> +    };
> +    H264RawSEIUserDataUnregistered *udu =
> +        &payload.payload.user_data_unregistered;
> +
> +    for (i = j = 0; j < 32 && data[i]; i++) {
> +        int c, v;
> +        c = data[i];
> +        if (c == '-') {
> +            continue;
> +        } else if (av_isxdigit(c)) {
> +            c = av_tolower(c);
> +            v = (c <= '9' ? c - '0' : c - 'a' + 10);
> +        } else {
> +            goto invalid_user_data;
> +        }
> +        if (i & 1)
> +            udu->uuid_iso_iec_11578[j / 2] |= v;
> +        else
> +            udu->uuid_iso_iec_11578[j / 2] = v << 4;
> +        ++j;
> +    }
> +    if (j == 32 && data[i] == '+') {
> +        size_t len = size - i - 1;
> +
> +        udu->data_ref = av_buffer_alloc(len + 1);
> +        if (!udu->data_ref) {
> +            err = AVERROR(ENOMEM);
> +            return err;

Simply return AVERROR(ENOMEM) directly.

> +        }
> +
> +        udu->data        = udu->data_ref->data;
> +        udu->data_length = len + 1;
> +        memcpy(udu->data, data + i + 1, len + 1);
> +
> +        err = ff_cbs_h264_add_sei_message(ctx->cbc, au, &payload);
> +        if (err < 0) {
> +            av_log(bsf, AV_LOG_ERROR, "Failed to add user data SEI "
> +                   "message to access unit.\n");
> +            return err;
> +        }
> +
> +    } else {
> +    invalid_user_data:
> +        av_log(bsf, AV_LOG_ERROR, "Invalid user data: "
> +               "must be \"UUID+string\".\n");
> +        err = AVERROR(EINVAL);
> +        return err;

Simply return AVERROR(EINVAL) directly.

> +    }
> +    return err;

Simply return 0 directly (also you don't need to initialize err any more).

> +}
> +
>  static int h264_metadata_update_side_data(AVBSFContext *bsf, AVPacket *pkt)
>  {
>      H264MetadataContext *ctx = bsf->priv_data;
> @@ -412,56 +471,7 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt)
>      // Only insert the SEI in access units containing SPSs, and also
>      // unconditionally in the first access unit we ever see.
>      if (ctx->sei_user_data && (has_sps || !ctx->done_first_au)) {
> -        H264RawSEIPayload payload = {
> -            .payload_type = H264_SEI_TYPE_USER_DATA_UNREGISTERED,
> -        };
> -        H264RawSEIUserDataUnregistered *udu =
> -            &payload.payload.user_data_unregistered;
> -
> -        for (i = j = 0; j < 32 && ctx->sei_user_data[i]; i++) {
> -            int c, v;
> -            c = ctx->sei_user_data[i];
> -            if (c == '-') {
> -                continue;
> -            } else if (av_isxdigit(c)) {
> -                c = av_tolower(c);
> -                v = (c <= '9' ? c - '0' : c - 'a' + 10);
> -            } else {
> -                goto invalid_user_data;
> -            }
> -            if (j & 1)
> -                udu->uuid_iso_iec_11578[j / 2] |= v;
> -            else
> -                udu->uuid_iso_iec_11578[j / 2] = v << 4;
> -            ++j;
> -        }
> -        if (j == 32 && ctx->sei_user_data[i] == '+') {
> -            size_t len = strlen(ctx->sei_user_data + i + 1);
> -
> -            udu->data_ref = av_buffer_alloc(len + 1);
> -            if (!udu->data_ref) {
> -                err = AVERROR(ENOMEM);
> -                goto fail;
> -            }
> -
> -            udu->data        = udu->data_ref->data;
> -            udu->data_length = len + 1;
> -            memcpy(udu->data, ctx->sei_user_data + i + 1, len + 1);
> -
> -            err = ff_cbs_h264_add_sei_message(ctx->cbc, au, &payload);
> -            if (err < 0) {
> -                av_log(bsf, AV_LOG_ERROR, "Failed to add user data SEI "
> -                       "message to access unit.\n");
> -                goto fail;
> -            }
> -
> -        } else {
> -        invalid_user_data:
> -            av_log(bsf, AV_LOG_ERROR, "Invalid user data: "
> -                   "must be \"UUID+string\".\n");
> -            err = AVERROR(EINVAL);
> -            goto fail;
> -        }
> +      write_sei_user_data(bsf, ctx->sei_user_data, strlen(ctx->sei_user_data));
>      }
>  
>      if (ctx->delete_filler) {
> @@ -604,6 +614,12 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt)
>          }
>      }
>  
> +    uint8_t *data;
> +    int size;

Mixed declaration and code. Furthermore, data should be const.

> +    if (data = av_packet_get_side_data(pkt, AV_PKT_DATA_SEI_USER, &size)) {
> +        write_sei_user_data(bsf, data, size);

You are adding this SEI unconditionally if the packet side data is
present. I think this should be explicitly enabled by the user via an
option. (Think about a situation where you add the relevant side data to
an AVPacket and use the h264_metadata bsf to add it to the packet's
payload (i.e. in-band). Then some further processing on this packet is
performed via code written by someone else; if this code happens to run
h264_metadata bsf on the packet, too, then this other code would
unintentionally add the SEI a second time.)

> +    }
> +
>      err = ff_cbs_write_packet(ctx->cbc, pkt, au);
>      if (err < 0) {
>          av_log(bsf, AV_LOG_ERROR, "Failed to write packet.\n");
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index 41485f4527..48e0ccbaf0 100644
> --- a/libavcodec/packet.h
> +++ b/libavcodec/packet.h
> @@ -282,6 +282,11 @@ enum AVPacketSideDataType {
>       */
>      AV_PKT_DATA_DOVI_CONF,
>  
> +    /**
> +     * This side data contains SEI unregistered Data.
> +     */
> +    AV_PKT_DATA_SEI_USER,
> +

You need to document the format of this AVPacketSideDataType. You simply
copied the format that h264_metadata already uses and that seems wrong
as this format has been designed in order to easily type it into the
command line. But for an API user a different format is better: The
first 16 byte are the UID and the rest (which needn't be zero-terminated
at all any more as it is just a buffer with a known length) is the user
data payload.

Furthermore, I wonder whether one should add a bit more semantics to the
side data: something that says whether the side data also exists in-band
or not. In the former case it would be safe to send this packets to
destinations that don't support out-of-band SEI messages.

- Andreas

On Tue, Jun 09, 2020 at 02:39:14PM +1000, Brad Hards wrote:
> >> --git a/libavcodec/packet.h b/libavcodec/packet.h index 
> >> 41485f4527..48e0ccbaf0 100644
> >> --- a/libavcodec/packet.h
> >> +++ b/libavcodec/packet.h
> >> @@ -282,6 +282,11 @@ enum AVPacketSideDataType {
> >>       */
> >>      AV_PKT_DATA_DOVI_CONF,
> >>  
> >> +    /**
> >> +     * This side data contains SEI unregistered Data.
> >> +     */
> >> +    AV_PKT_DATA_SEI_USER,
> >> +
> >
> >it's public header
> 
> I don't understand this comment. What are you expecting as the change here?

It's API change for public header, so I think it's necessary to update doc/APIchanges 
and bump version. 
Also SEI unregisted data is h264 and h265 codec SEI data, I haven't clear why it's packet side
data yet. I add the SEI user data for frame level before, maybe you can check it:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200317105409.2795-1-lance.lmwang@gmail.com/

> 
> Brad
>

On Mon, 1 Jun 2020 at 22:23, Daniel Loman <dloman@toyon.com> wrote:

> Added seperate side data field to allow adding per packet side data
> message to support MISB 604 encoding
>

Are you aware that this is not going to be frame accurate for the non-SPS
frame because of reordering?

Kieran

From 35034aee293cfd644ce628d3fd932ab7a0efbc10 Mon Sep 17 00:00:00 2001
From: Daniel Loman <dloman@toyon.com>
Date: Tue, 9 Jun 2020 11:10:12 -0700
Subject: [PATCH 1/2] added sei side data field
To: ffmpeg-devel@ffmpeg.org

---
 Changelog             | 1 +
 libavcodec/avpacket.c | 1 +
 libavcodec/packet.h   | 6 ++++++
 libavcodec/version.h  | 2 +-
 4 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Changelog b/Changelog
index 711c843b99..631958015a 100644
--- a/Changelog
+++ b/Changelog
@@ -75,6 +75,7 @@ version <next>:
 - PFM decoder
 - dblur video filter
 - Real War KVAG muxer
+- added sei side data field to AVPacket 
 
 
 version 4.2:
diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index 033f2d8f26..a530dc6779 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -395,6 +395,7 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type)
     case AV_PKT_DATA_A53_CC:                     return "A53 Closed Captions";
     case AV_PKT_DATA_ENCRYPTION_INIT_INFO:       return "Encryption initialization data";
     case AV_PKT_DATA_ENCRYPTION_INFO:            return "Encryption info";
+    case AV_PKT_DATA_SEI_USER:                   return "SEI unregistered data";
     case AV_PKT_DATA_AFD:                        return "Active Format Description data";
     case AV_PKT_DATA_PRFT:                       return "Producer Reference Time";
     case AV_PKT_DATA_ICC_PROFILE:                return "ICC Profile";
diff --git a/libavcodec/packet.h b/libavcodec/packet.h
index 41485f4527..8c62c467dc 100644
--- a/libavcodec/packet.h
+++ b/libavcodec/packet.h
@@ -282,6 +282,12 @@ enum AVPacketSideDataType {
      */
     AV_PKT_DATA_DOVI_CONF,
 
+    /**
+     * This side data contains SEI unregistered Data.
+     * first 17 bytes are UID and + the rest are non zero terminated fixed length bytes
+     */
+    AV_PKT_DATA_SEI_USER,
+
     /**
      * The number of side data types.
      * This is not part of the public API/ABI in the sense that it may
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 524fbc3b11..4e2cc5db92 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -29,7 +29,7 @@
 
 #define LIBAVCODEC_VERSION_MAJOR  58
 #define LIBAVCODEC_VERSION_MINOR  90
-#define LIBAVCODEC_VERSION_MICRO 100
+#define LIBAVCODEC_VERSION_MICRO 101
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
                                                LIBAVCODEC_VERSION_MINOR, \

KK>>Are you aware that this is not going to be frame accurate for the non-SPS
KK>>frame because of reordering?

reordering of the frame in terms of pts versus dts?

these are attached to the AVPackets not AVFrame. They data should correspond with the pts of the packet. unless i am confused with what youre saying

On Tue, 9 Jun 2020 at 22:26, Daniel Loman <dloman@toyon.com> wrote:

> KK>>Are you aware that this is not going to be frame accurate for the
> non-SPS
> KK>>frame because of reordering?
>
> reordering of the frame in terms of pts versus dts?
>
> these are attached to the AVPackets not AVFrame. They data should
> correspond with the pts of the packet. unless i am confused with what youre
> saying
>

 Ok. as long as you are aware PTS may not be in order.

Kieran

The side data that is added is a precision time stamp (MISB ST0603), which may or may not relate to the presentation time stamp. 

Unfortunate overlap of acronym.

Brad

Yes. I think what he was asking is are you sure you want to add this to the AVPacket which is encoded data and will be ordered in decode time not presentation time. The precision time stamp is a separate clock but should be more closely related to the presentation time stamp (assumming its there). It looks like there are several merge requests for adding side data to the AVFrame https://patchwork.ffmpeg.org/project/ffmpeg/patch/1591793766-15844-1-git-send-email-lance.lmwang@gmail.com/ I am not sure if this is related or conincidental

Dan