diff mbox

[FFmpeg-devel,2/2] decklink: Add support for output of Active Format Description (AFD)

Message ID 20180822195336.90799-3-dheitmueller@ltnglobal.com
State Superseded
Headers show

Commit Message

Devin Heitmueller Aug. 22, 2018, 7:53 p.m. UTC
Implement support for including AFD in decklink output.  This
includes making sure the AFD data is preserved when going from
an AVFrame to a V210 packet (needed for 10-bit support).

Updated to reflect feedback from Marton Balint <cus@passwd.hu>,
Carl Eugen Hoyos <ceffmpeg@gmail.com> and Aaron Levinson
<alevinsn_dev@levland.net>.

Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
---
 libavcodec/avcodec.h         |  6 +++++
 libavcodec/v210enc.c         |  8 +++++++
 libavdevice/decklink_enc.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 66 insertions(+), 2 deletions(-)

Comments

Marton Balint Aug. 26, 2018, 3:34 p.m. UTC | #1
On Wed, 22 Aug 2018, Devin Heitmueller wrote:

> Implement support for including AFD in decklink output.  This
> includes making sure the AFD data is preserved when going from
> an AVFrame to a V210 packet (needed for 10-bit support).
>
> Updated to reflect feedback from Marton Balint <cus@passwd.hu>,
> Carl Eugen Hoyos <ceffmpeg@gmail.com> and Aaron Levinson
> <alevinsn_dev@levland.net>.
>
> Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
> ---
> libavcodec/avcodec.h         |  6 +++++
> libavcodec/v210enc.c         |  8 +++++++
> libavdevice/decklink_enc.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 31e50d5a94..192e15746d 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1362,6 +1362,12 @@ enum AVPacketSideDataType {
>     AV_PKT_DATA_ENCRYPTION_INFO,
>
>     /**
> +     * Active Format Description data consisting of a single byte as specified
> +     * in ETSI TS 101 154 using AVActiveFormatDescription enum.
> +     */
> +    AV_PKT_DATA_AFD,
> +
> +    /**
>      * The number of side data types.
>      * This is not part of the public API/ABI in the sense that it may
>      * change when new side data types are added.

Such addition requires a libavcodec minor version bump and maybe an entry 
in doc/APIChanges.

> diff --git a/libavcodec/v210enc.c b/libavcodec/v210enc.c
> index b9dcf9a672..b024806d0b 100644
> --- a/libavcodec/v210enc.c
> +++ b/libavcodec/v210enc.c
> @@ -242,6 +242,14 @@ static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
>         memcpy(buf, side_data->data, side_data->size);
>     }
> 
> +    side_data = av_frame_get_side_data(pic, AV_FRAME_DATA_AFD);
> +    if (side_data && side_data->size) {
> +        uint8_t *buf = av_packet_new_side_data(pkt, AV_PKT_DATA_AFD, side_data->size);
> +        if (!buf)
> +            return AVERROR(ENOMEM);
> +        memcpy(buf, side_data->data, side_data->size);
> +    }
> +
>     pkt->flags |= AV_PKT_FLAG_KEY;
>     *got_packet = 1;
>     return 0;

A separate patch from decklink is preferred here as well for the API and 
v210enc addition.

> diff --git a/libavdevice/decklink_enc.cpp b/libavdevice/decklink_enc.cpp
> index de7758cb91..2d03d33554 100644
> --- a/libavdevice/decklink_enc.cpp
> +++ b/libavdevice/decklink_enc.cpp
> @@ -303,7 +303,8 @@ av_cold int ff_decklink_write_trailer(AVFormatContext *avctx)
> 
> #if CONFIG_LIBKLVANC
> static int decklink_construct_vanc(AVFormatContext *avctx, struct decklink_ctx *ctx,
> -                                   AVPacket *pkt, decklink_frame *frame)
> +                                   AVPacket *pkt, decklink_frame *frame,
> +                                   AVStream *st)
> {
>     struct klvanc_line_set_s vanc_lines = { 0 };
>     int ret, size, i;
> @@ -363,6 +364,55 @@ static int decklink_construct_vanc(AVFormatContext *avctx, struct decklink_ctx *
>         }
>     }
> 
> +    data = av_packet_get_side_data(pkt, AV_PKT_DATA_AFD, &size);
> +    if (data) {

if (data && size)

> +        struct klvanc_packet_afd_s *pkt;

Maybe pkt is not the best name, can be confused with the AVPacket pkt...

> +        uint16_t *afd;
> +        uint16_t len;
> +
> +        ret = klvanc_create_AFD(&pkt);
> +        if (ret) {
> +            for (i = 0; i < vanc_lines.num_lines; i++)
> +                klvanc_line_free(vanc_lines.lines[i]);
> +            return AVERROR(ENOMEM);

As I mentioned in the previous patch, replace these with something like:

ret = AVERROR(ENOMEM);
goto out;

> +        }
> +
> +        ret = klvanc_set_AFD_val(pkt, data[0]);
> +        if (ret) {
> +            av_log(avctx, AV_LOG_ERROR, "Invalid AFD value specified: %d\n",
> +                   data[0]);
> +            klvanc_destroy_AFD(pkt);
> +            for (i = 0; i < vanc_lines.num_lines; i++)
> +                klvanc_line_free(vanc_lines.lines[i]);
> +            return AVERROR(EINVAL);

here

> +        }
> +
> +        /* FIXME: Should really rely on the coded_width but seems like that
> +           is not accessible to libavdevice outputs */
> +        if (av_cmp_q((AVRational) {st->codecpar->width, st->codecpar->height}, (AVRational) {4, 3}) == 1)
> +            pkt->aspectRatio = ASPECT_16x9;
> +        else
> +            pkt->aspectRatio = ASPECT_4x3;
> +
> +        ret = klvanc_convert_AFD_to_words(pkt, &afd, &len);
> +        klvanc_destroy_AFD(pkt);
> +        if (ret) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed converting AFD packet to words\n");
> +            for (i = 0; i < vanc_lines.num_lines; i++)
> +                klvanc_line_free(vanc_lines.lines[i]);
> +            return AVERROR(ENOMEM);

here

> +        }
> +
> +        ret = klvanc_line_insert(ctx->vanc_ctx, &vanc_lines, afd, len, 12, 0);
> +        free(afd);
> +        if (ret) {
> +            av_log(avctx, AV_LOG_ERROR, "VANC line insertion failed\n");
> +            for (i = 0; i < vanc_lines.num_lines; i++)
> +                klvanc_line_free(vanc_lines.lines[i]);
> +            return AVERROR(ENOMEM);

and here.

> +        }
> +    }
> +
>     IDeckLinkVideoFrameAncillary *vanc;
>     int result = ctx->dlo->CreateAncillaryData(bmdFormat10BitYUV, &vanc);
>     if (result != S_OK) {
> @@ -457,7 +507,7 @@ static int decklink_write_video_packet(AVFormatContext *avctx, AVPacket *pkt)
>         frame = new decklink_frame(ctx, avpacket, st->codecpar->codec_id, ctx->bmd_height, ctx->bmd_width);
> 
> #if CONFIG_LIBKLVANC
> -        ret = decklink_construct_vanc(avctx, ctx, pkt, frame);
> +        ret = decklink_construct_vanc(avctx, ctx, pkt, frame, st);
>         if (ret)
>             av_log(avctx, AV_LOG_ERROR, "Failed to construct VANC\n");
> #endif

Thanks,
Marton
Vittorio Giovara Aug. 27, 2018, 4:25 p.m. UTC | #2
On Wed, Aug 22, 2018 at 9:53 PM, Devin Heitmueller <
dheitmueller@ltnglobal.com> wrote:

> Implement support for including AFD in decklink output.  This
> includes making sure the AFD data is preserved when going from
> an AVFrame to a V210 packet (needed for 10-bit support).
>
> Updated to reflect feedback from Marton Balint <cus@passwd.hu>,
> Carl Eugen Hoyos <ceffmpeg@gmail.com> and Aaron Levinson
> <alevinsn_dev@levland.net>.
>
> Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
> ---
>  libavcodec/avcodec.h         |  6 +++++
>  libavcodec/v210enc.c         |  8 +++++++
>  libavdevice/decklink_enc.cpp | 54 ++++++++++++++++++++++++++++++
> ++++++++++++--
>  3 files changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 31e50d5a94..192e15746d 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1362,6 +1362,12 @@ enum AVPacketSideDataType {
>      AV_PKT_DATA_ENCRYPTION_INFO,
>
>      /**
> +     * Active Format Description data consisting of a single byte as
> specified
> +     * in ETSI TS 101 154 using AVActiveFormatDescription enum.
> +     */
> +    AV_PKT_DATA_AFD,
> +
> +    /**
>       * The number of side data types.
>       * This is not part of the public API/ABI in the sense that it may
>       * change when new side data types are added.
>

I think you should add an entry in ff_decode_frame_props() so that pkt side
data can propagate to frame side data
Devin Heitmueller Aug. 27, 2018, 4:36 p.m. UTC | #3
Hello Vittorio,

Thanks for the feedback.

> 
> I think you should add an entry in ff_decode_frame_props() so that pkt side data can propagate to frame side data 
> -- 
> Vittorio

I’ve got a whole patch series related to capture of AFD from decklink and getting it through the pipeline (to be encoded by libx264), which includes a patch which makes the entry you have suggested.

I’ve got about 40 ffmpeg related patches I am trying to get merged upstream (many dealing with side data on both the capture and output side), so for the moment I’m focused on getting the first few output related patches merged.  In some cases it’s been more energy carving up the patch series in different ways to get it merged than the actual engineering required to do the work in the first place.

Thanks,

Devin
Devin Heitmueller Aug. 27, 2018, 4:38 p.m. UTC | #4
> On Aug 26, 2018, at 11:34 AM, Marton Balint <cus@passwd.hu> wrote:
> 
> 

Hello Marton,

Ok, I’ll take another pass and send an updated patch.

Devin

---
Devin Heitmueller - LTN Global Communications
dheitmueller@ltnglobal.com
diff mbox

Patch

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 31e50d5a94..192e15746d 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -1362,6 +1362,12 @@  enum AVPacketSideDataType {
     AV_PKT_DATA_ENCRYPTION_INFO,
 
     /**
+     * Active Format Description data consisting of a single byte as specified
+     * in ETSI TS 101 154 using AVActiveFormatDescription enum.
+     */
+    AV_PKT_DATA_AFD,
+
+    /**
      * The number of side data types.
      * This is not part of the public API/ABI in the sense that it may
      * change when new side data types are added.
diff --git a/libavcodec/v210enc.c b/libavcodec/v210enc.c
index b9dcf9a672..b024806d0b 100644
--- a/libavcodec/v210enc.c
+++ b/libavcodec/v210enc.c
@@ -242,6 +242,14 @@  static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
         memcpy(buf, side_data->data, side_data->size);
     }
 
+    side_data = av_frame_get_side_data(pic, AV_FRAME_DATA_AFD);
+    if (side_data && side_data->size) {
+        uint8_t *buf = av_packet_new_side_data(pkt, AV_PKT_DATA_AFD, side_data->size);
+        if (!buf)
+            return AVERROR(ENOMEM);
+        memcpy(buf, side_data->data, side_data->size);
+    }
+
     pkt->flags |= AV_PKT_FLAG_KEY;
     *got_packet = 1;
     return 0;
diff --git a/libavdevice/decklink_enc.cpp b/libavdevice/decklink_enc.cpp
index de7758cb91..2d03d33554 100644
--- a/libavdevice/decklink_enc.cpp
+++ b/libavdevice/decklink_enc.cpp
@@ -303,7 +303,8 @@  av_cold int ff_decklink_write_trailer(AVFormatContext *avctx)
 
 #if CONFIG_LIBKLVANC
 static int decklink_construct_vanc(AVFormatContext *avctx, struct decklink_ctx *ctx,
-                                   AVPacket *pkt, decklink_frame *frame)
+                                   AVPacket *pkt, decklink_frame *frame,
+                                   AVStream *st)
 {
     struct klvanc_line_set_s vanc_lines = { 0 };
     int ret, size, i;
@@ -363,6 +364,55 @@  static int decklink_construct_vanc(AVFormatContext *avctx, struct decklink_ctx *
         }
     }
 
+    data = av_packet_get_side_data(pkt, AV_PKT_DATA_AFD, &size);
+    if (data) {
+        struct klvanc_packet_afd_s *pkt;
+        uint16_t *afd;
+        uint16_t len;
+
+        ret = klvanc_create_AFD(&pkt);
+        if (ret) {
+            for (i = 0; i < vanc_lines.num_lines; i++)
+                klvanc_line_free(vanc_lines.lines[i]);
+            return AVERROR(ENOMEM);
+        }
+
+        ret = klvanc_set_AFD_val(pkt, data[0]);
+        if (ret) {
+            av_log(avctx, AV_LOG_ERROR, "Invalid AFD value specified: %d\n",
+                   data[0]);
+            klvanc_destroy_AFD(pkt);
+            for (i = 0; i < vanc_lines.num_lines; i++)
+                klvanc_line_free(vanc_lines.lines[i]);
+            return AVERROR(EINVAL);
+        }
+
+        /* FIXME: Should really rely on the coded_width but seems like that
+           is not accessible to libavdevice outputs */
+        if (av_cmp_q((AVRational) {st->codecpar->width, st->codecpar->height}, (AVRational) {4, 3}) == 1)
+            pkt->aspectRatio = ASPECT_16x9;
+        else
+            pkt->aspectRatio = ASPECT_4x3;
+
+        ret = klvanc_convert_AFD_to_words(pkt, &afd, &len);
+        klvanc_destroy_AFD(pkt);
+        if (ret) {
+            av_log(avctx, AV_LOG_ERROR, "Failed converting AFD packet to words\n");
+            for (i = 0; i < vanc_lines.num_lines; i++)
+                klvanc_line_free(vanc_lines.lines[i]);
+            return AVERROR(ENOMEM);
+        }
+
+        ret = klvanc_line_insert(ctx->vanc_ctx, &vanc_lines, afd, len, 12, 0);
+        free(afd);
+        if (ret) {
+            av_log(avctx, AV_LOG_ERROR, "VANC line insertion failed\n");
+            for (i = 0; i < vanc_lines.num_lines; i++)
+                klvanc_line_free(vanc_lines.lines[i]);
+            return AVERROR(ENOMEM);
+        }
+    }
+
     IDeckLinkVideoFrameAncillary *vanc;
     int result = ctx->dlo->CreateAncillaryData(bmdFormat10BitYUV, &vanc);
     if (result != S_OK) {
@@ -457,7 +507,7 @@  static int decklink_write_video_packet(AVFormatContext *avctx, AVPacket *pkt)
         frame = new decklink_frame(ctx, avpacket, st->codecpar->codec_id, ctx->bmd_height, ctx->bmd_width);
 
 #if CONFIG_LIBKLVANC
-        ret = decklink_construct_vanc(avctx, ctx, pkt, frame);
+        ret = decklink_construct_vanc(avctx, ctx, pkt, frame, st);
         if (ret)
             av_log(avctx, AV_LOG_ERROR, "Failed to construct VANC\n");
 #endif