[FFmpeg-devel] [PATCH 1/2] avdevice/decklink_dec: add support for decoding teletext from 10bit ancillary data

[Marton Balint]

On Mon, 3 Jul 2017, Aaron Levinson wrote:

>> +static uint8_t* teletext_data_unit_from_vbi_data_10bit(int line, uint8_t 
> *src, uint8_t *tgt)
>> +{
>> +    uint8_t y[720];
>> +    uint8_t *py = y;
>> +    uint8_t *pend = y + 720;
>> +    while (py < pend) {
>> +        *py++ = (src[1] >> 4) + ((src[2] & 15) << 4);
>> +        *py++ = (src[4] >> 2) + ((src[5] & 3 ) << 6);
>> +        *py++ = (src[6] >> 6) + ((src[7] & 63) << 2);
>> +        src += 8;
>> +    }
>
> It would seem that the purpose of this code is to convert between the 
> Blackmagic 10-bit format (v210, according to the Blackmagic 
> documentation) and YUV420 (which is presumably easier than converting to 
> UYVY).  Why not use a color converter for this?

Because I only need the luma component and only in 8 bit, so getting 
anything else would be a waste of resources. Also the code is short enough 
and more readable than factorizing something from v210dec.

> Looking through the 
> ffmpeg code base, I see that v210 is handled using a decoder/encoder and 
> not as a pixel format, which seems odd, but maybe there is some way to 
> take advantage of that support instead of doing it yourself.  At the 
> very least, it seems reasonable to add a comment explaining what this 
> code is doing.

ok for the comment.

>
> Also, I understand that this is being done because libzvbi doesn't 
> support the v210 pixel format (which would be useful to explain in a 
> comment), but I have to wonder if the conversion between the 10-bit v210 
> pixel format and the 8-bit YUV420 pixel format will result in a loss of 
> data that is relevant for teletext.  If you've accommodated that 
> possibility, it would be good to describe in a comment.

I understand your concern, but for teletext you decode roughly 0.5 bit of 
information from a 8 bit value. So even using 8bit is a lot more than what 
you need. I will add a comment about this.

>>   #if CONFIG_LIBZVBI
>> -        if (!no_video && ctx->teletext_lines && 
> videoFrame->GetPixelFormat() == bmdFormat8BitYUV && videoFrame->GetWidth() == 
> 720) {
>> +        if (!no_video && ctx->teletext_lines) {
>>               IDeckLinkVideoFrameAncillary *vanc;
>>               AVPacket txt_pkt;
>>               uint8_t txt_buf0[1611]; // max 35 * 46 bytes decoded teletext 
> lines + 1 byte data_identifier
>> @@ -368,16 +387,21 @@ HRESULT 
> decklink_input_callback::VideoInputFrameArrived(
>>               if (videoFrame->GetAncillaryData(&vanc) == S_OK) {
>>                   int i;
>>                   int64_t line_mask = 1;
>> +                BMDPixelFormat vanc_format = vanc->GetPixelFormat();
>>                   txt_buf[0] = 0x10;    // data_identifier - EBU_data
>>                   txt_buf++;
>> -                for (i = 6; i < 336; i++, line_mask <<= 1) {
>> -                    uint8_t *buf;
>> -                    if ((ctx->teletext_lines & line_mask) && 
> vanc->GetBufferForVerticalBlankingLine(i, (void**)&buf) == S_OK) {
>> -                        if (teletext_data_unit_from_vbi_data(i, buf, 
> txt_buf) >= 0)
>> -                            txt_buf += 46;
>> +                if (videoFrame->GetWidth() == 720 && (vanc_format == 
> bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) {
>
> According to the documentation in indevs.texi, the teletext support only 
> works for SD PAL.  Yet, this check doesn't isolate things just to SD 
> PAL.  I know that this issue was preexisting (although you have moved 
> the check to a different place in the code), but if you want to restrict 
> this code to SD PAL, then the best way is to check if the BMDDisplayMode 
> is bmdModePAL.  If you want to base things on the width/height, you 
> could check for a width of 720 and a height of 576, although this could 
> indicate either bmdModePAL or bmdModePALp (although there may be no 
> Blackmagic devices that support bmdModePALp on input).  Just checking 
> for a width of 720 includes NTSC, NTSCp, PAL, and PALp.

Ok.

>
>> +                    for (i = 6; i < 336; i++, line_mask <<= 1) {
>> +                        uint8_t *buf;
>> +                        if ((ctx->teletext_lines & line_mask) && 
> vanc->GetBufferForVerticalBlankingLine(i, (void**)&buf) == S_OK) {
>> +                            if (vanc_format == bmdFormat8BitYUV)
>> +                                txt_buf = 
> teletext_data_unit_from_vbi_data(i, buf, txt_buf, VBI_PIXFMT_UYVY);
>> +                            else
>> +                                txt_buf = 
> teletext_data_unit_from_vbi_data_10bit(i, buf, txt_buf);
>
> Since you already know if the format is 10-bit or 8-bit prior to 
> entering the for loop, putting the check here may result in an 
> unnecessary branch every loop iteration, depending on how the compiler 
> optimizes the code.  So, for efficiency, it would make more sense to 
> move the if check to earlier, prior to executing the for loop, but this 
> will end up causing you to effectively duplicate the for loop code the 
> way it is currently written.
>

The overhead is negligable (35 if-s per frame) and I find this more 
readable than duplicating the whole loop.

>> +                        }
>> +                        if (i == 22)
>> +                            i = 317;
>>                       }
>> -                    if (i == 22)
>> -                        i = 317;
>>                   }
>>                   vanc->Release();
>>                   if (txt_buf - txt_buf0 > 1) {
>> 
>
> Overall, looks good.  I'll review the second patch in a bit.

Thanks, I will send a v2.

Regards,
Marton