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

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).

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

On Thu, 16 Nov 2017, 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).
>
> Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
> ---
> libavcodec/avcodec.h         |  6 ++++++
> libavcodec/v210enc.c         | 11 +++++++++++
> libavdevice/decklink_enc.cpp | 41 +++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 442b558..6981f07 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1327,6 +1327,12 @@ enum AVPacketSideDataType {
>     AV_PKT_DATA_A53_CC,
>
>     /**
> +     * 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 elements (in fact a bit more than it).
>      * 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 a825c03..c28f7b1 100644
> --- a/libavcodec/v210enc.c
> +++ b/libavcodec/v210enc.c
> @@ -245,6 +245,17 @@ static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
>         }
>     }
> 
> +    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) {
> +            memcpy(buf, side_data->data, side_data->size);
> +        } else {
> +            av_log(avctx, AV_LOG_ERROR, "Unable to allocate afd side data\n");

Same here, you can skip the error message for enomem.

> +            return AVERROR(ENOMEM);
> +        }
> +    }
> +
>     pkt->flags |= AV_PKT_FLAG_KEY;
>     *got_packet = 1;
>     return 0;
> diff --git a/libavdevice/decklink_enc.cpp b/libavdevice/decklink_enc.cpp
> index 0dcbe79..070bfad 100644
> --- a/libavdevice/decklink_enc.cpp
> +++ b/libavdevice/decklink_enc.cpp
> @@ -280,7 +280,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;
> @@ -334,6 +335,42 @@ 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 != 0)
> +            return AVERROR(ENOMEM);
> +
> +        ret = klvanc_set_AFD_val(pkt, data[0]);
> +        if (ret != 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Invalid AFD value specified: %d\n",
> +                   data[0]);
> +            klvanc_destroy_AFD(pkt);
> +            return AVERROR(EINVAL);
> +        }
> +
> +        /* FIXME: Should really rely on the coded_width but seems like that
> +           is not accessible to libavdevice outputs */
> +        if ((st->codecpar->width == 1280 && st->codecpar->height == 720) ||
> +            (st->codecpar->width == 1920 && st->codecpar->height == 1080))
> +            pkt->aspectRatio = ASPECT_16x9;
> +        else
> +            pkt->aspectRatio = ASPECT_4x3;

Does this work for SD 16x9? Shouldn't you also use st->sample_aspect_ratio 
with some rounding to handle 704x576 16:9 and such mess?

> +
> +        klvanc_convert_AFD_to_words(pkt, &afd, &len);

missing error check

> +        klvanc_destroy_AFD(pkt);
> +
> +        ret = klvanc_line_insert(ctx->vanc_ctx, &vanc_lines, afd, len, 12, 0);
> +        if (ret != 0) {
> +            av_log(avctx, AV_LOG_ERROR, "VANC line insertion failed\n");
> +            return AVERROR(ENOMEM);
> +        }
> +    }
> +
>     IDeckLinkVideoFrameAncillary *vanc;
>     int result = ctx->dlo->CreateAncillaryData(bmdFormat10BitYUV, &vanc);
>     if (result != S_OK) {
> @@ -429,7 +466,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 != 0) {
>             av_log(avctx, AV_LOG_ERROR, "Failed to construct VANC\n");
>         }

Regards,
Marton

Hi Marton,

Comments inline.

>> +    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 != 0)
>> +            return AVERROR(ENOMEM);
>> +
>> +        ret = klvanc_set_AFD_val(pkt, data[0]);
>> +        if (ret != 0) {
>> +            av_log(avctx, AV_LOG_ERROR, "Invalid AFD value specified: %d\n",
>> +                   data[0]);
>> +            klvanc_destroy_AFD(pkt);
>> +            return AVERROR(EINVAL);
>> +        }
>> +
>> +        /* FIXME: Should really rely on the coded_width but seems like that
>> +           is not accessible to libavdevice outputs */
>> +        if ((st->codecpar->width == 1280 && st->codecpar->height == 720) ||
>> +            (st->codecpar->width == 1920 && st->codecpar->height == 1080))
>> +            pkt->aspectRatio = ASPECT_16x9;
>> +        else
>> +            pkt->aspectRatio = ASPECT_4x3;
> 
> Does this work for SD 16x9? Shouldn't you also use st->sample_aspect_ratio with some rounding to handle 704x576 16:9 and such mess?

Bear in mind that the “aspectRatio” field in question isn’t what the video should ultimately be rendered in.  It’s just the aspect ratio of the source video.  For cases where you’re doing SD with 16x9, the source video itself is in 4x3 aspect ratio, but the expected result should be in 16x9.  The fact that the video should be rendered in 16x9 is controlled by the call to klvanc_set_AFD_val() further up in the function.  SAR/PAR, etc aren’t a consideration for what drives the aspectRatio field in AFD.

In short, this routine could probably be replaced with something that just divides pixelwidth/pixelheight and determines whether it’s equal to 1.77 or some smaller value.

I know it’s counter-intuitive to have a detailed aspect ratio description with something like 16 possible values, and then to also have a single bit which indicates whether the original source video is “16x9” or “4x3” when any downstream device can just look at the real pixel width/height of the video.  Complain to the ATSC, I guess.  :-)

Devin