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

[Devin Heitmueller]

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