[FFmpeg-devel] [PATCH] added sei side data
Andreas Rheinhardt
andreas.rheinhardt at gmail.com
Tue Jun 9 16:18:03 EEST 2020
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
More information about the ffmpeg-devel
mailing list