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

[Aaron Levinson]

On 12/29/2017 1:20 PM, Devin Heitmueller wrote:
> 
>> On Dec 29, 2017, at 4:09 PM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>>
>> 2017-12-29 22:02 GMT+01:00 Devin Heitmueller <dheitmueller at ltnglobal.com>:
>>>
>>>> On Dec 29, 2017, at 3:48 PM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>>>>
>>>> 2017-12-29 19:12 GMT+01:00 Devin Heitmueller <dheitmueller at ltnglobal.com>:
>>>>
>>>>> +        /* 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;
>>>>
>>>> I most likely won't use this (and I have never seen a decklink card)
>>>> so please feel free to ignore:
>>>> Similar code has caused some trouble with mxf files, is there
>>>> really no saner solution? Like comparing what the actual aspect
>>>> ratio is more similar to? Is SAR really always 1 for decklink?
>>>> ("All the world's a VAX.")
>>>
>>> So this is definitely a confusing block of code, and you aren’t the first one to
>>> ask about it (there were questions in the last round of review as well). The
>>> aspect ratio referred to here is actually of the original coded video - not how
>>> it’s supposed to be displayed.  Hence, for example, 720x480 in widescreen
>>> with a non-square PAR would still have the aspect ratio set to 4x3, since
>>> that particular field describes the coded video (i.e. *not* how it’s supposed
>>> to be rendered).
>>
>> And a resolution of 1024x576 does not exist for decklink?
>>
>> What about 1920x1088?
>>
> 
> The Blackmagic cards have a fairly constrained set of modes which are supported (see the definition of BMDDisplayMode in the Decklink SDK documentation).  Neither of the modes you mentioned are available for output, although I’m not sure what st->codecpar->width would be set to if swscale is used between the decoder and the output.
> 
> Bear in mind, I’m not particularly happy with how that block of code is done either (which is why I marked it with a FIXME), but it’s probably good enough for 95% of use cases.
> 
> Devin

Technically, there are a number of 2K and 4K video modes supported by 
some DeckLink cards that have a 16x9 aspect ratio as well.  This code 
would treat such video modes at 4:3.  The various 4K DCI video modes 
supported by Blackmagic use an aspect ratio of 256:135, although perhaps 
it is sufficient to treat those as 16:9.  Perhaps the logic should be, 
anything with a width of 1280 or greater (or a height of 720 or greater) 
is 16:9--everything else can be treated as 4:3.

Admittedly, libklvanc may not support VANC for 2K and 4K video anyway, 
but the Blackmagic devices do support it, so if you want to rule those 
out, then some explicit code to only support up to 720p/1080i (maybe 
1080p?) probably ought to be added.

On a separate note, based on a similar review I did awhile back, if this 
set of patches only supports a specific set of video modes, then there 
probably ought to be checks to make sure the code changes are only 
exercised for those video modes.

Aaron Levinson