[FFmpeg-devel] [PATCH 2/2] avdevice/decklink_dec: add support for receiving op47 teletext

[Marton Balint]

On Mon, 3 Jul 2017, Aaron Levinson wrote:

>> +
>> +This option is a bitmask of the SD VBI lines captured, specifically lines 
> 6 to
>
> Probably should be "SD PAL VBI lines" to make it clear that NTSC is not 
> supported.

ok.

>
>> +22, and lines 318 to 335. Line 6 is the LSB in the mask. Selected lines 
> which
>> +do not contain teletext information will be ignored. You can use the 
> special
>> + at option{all} constant to select all possible lines, or @option{standard} 
> to
>> +skip lines 6, 318 and 319, which are not compatible with all receivers.
>> +
>> +For SD sources ffmpeg needs to be compiled with @code{--enable-libzvbi}. 
> For HD
>> +sources on older (pre-4K) DeckLink card models you have to capture in 10 
> bit
>> +mode.
>
> Would be good to indicate that the bit mask is ignored for HD sources.

Actually it is not. Any HD line can contain an OP47 packet referencing any 
SD line, the bitmask is checked against the decoded source line number 
from OP47.

>
> The documentation indicates that both SD PAL and HD sources are 
> supported, but an examination of the changes indicates that only some HD 
> sources are supported.  Specifically, for HD sources to work, it expects 
> a width of 1920.  This would cover both 1080i and 1080p, but it doesn't 
> cover 720p, with is also an HD video mode.  My guess is that the code 
> has probably only been tested with 1080i as well, and in that case, it 
> would make sense to only specify 1080i in the documentation.  Further, 
> since the original code only supports SD PAL, I would suspect that the 
> latest code has only been tested using "PAL" frame rates at 1080i, i.e. 
> 1080i50.  If it is unclear if the code will also work with 1080i59.94 
> and 1080i60, then it would be best to only support 1080i50.

1080i/p/59.94/60 should work all the same. For 720p different VANC lines 
might be needed. I will change the HD references to "Full HD" so it will 
be more clear.

>
> Also, should have a comma after "For SD sources" and after "For HD sources".

Ok.

>> +static uint8_t* teletext_data_unit_from_vanc_data(uint8_t *src, uint8_t 
> *tgt, int64_t wanted_lines)
>> +{
>> +    int y[1920];
>> +    int *py = y;
>> +    int *pend = y + 1920;
>> +    while (py < pend) {
>> +        *py++ = (src[1] >> 2) + ((src[2] & 15) << 6);
>> +        *py++ =  src[4]       + ((src[5] &  3) << 8);
>> +        *py++ = (src[6] >> 4) + ((src[7] & 63) << 4);
>> +        src += 8;
>> +    }
>
> My comments from the last review pertain to this code.  Plus, now this 
> code is duplicated, so it would make sense to consolidate it to a new 
> function.

Not exaclty the same, because I need the 10 bit data here. Only thing 
I could do is use some DEFINE magic to factorize this.

>
>> +    py = y;
>> +    while (py < pend - 6) {
>> +        if (py[0] == 0 && py[1] == 0x3ff && py[2] == 0x3ff) { 
> // ancilliary data flag
>
> "ancilliary" -> "ancillary" in the comment

ok.

>> -#if CONFIG_LIBZVBI
>>           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
>> +            uint8_t txt_buf0[3531]; // 35 * 46 bytes decoded teletext 
> lines + 1 byte data_identifier + 1920 bytes OP47 decode buffer
>
> It is a little hard to follow this code, but it would seem that a max of 
> 1611 or 1920 bytes would be needed (so 1920), not 1611 + 1920 bytes.

Not exactly, in the HD case 1611 bytes are reserved for existing decoded 
packets, 1920 bytes are reserved for the teletext decoded from the next 
VANC line, which might contain multiple teletext lines.

>
>>               uint8_t *txt_buf = txt_buf0;
>>
>>               if (videoFrame->GetAncillaryData(&vanc) == S_OK) {
>> @@ -390,6 +478,7 @@ HRESULT 
> decklink_input_callback::VideoInputFrameArrived(
>>                   BMDPixelFormat vanc_format = vanc->GetPixelFormat();
>>                   txt_buf[0] = 0x10;    // data_identifier - EBU_data
>>                   txt_buf++;
>> +#if CONFIG_LIBZVBI
>>                   if (videoFrame->GetWidth() == 720 && (vanc_format == 
> bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) {
>>                       for (i = 6; i < 336; i++, line_mask <<= 1) {
>>                           uint8_t *buf;
>> @@ -403,6 +492,20 @@ HRESULT 
> decklink_input_callback::VideoInputFrameArrived(
>>                               i = 317;
>>                       }
>>                   }
>> +#endif
>> +                if (videoFrame->GetWidth() == 1920 && vanc_format == 
> bmdFormat10BitYUV) {
>
> As written, this would support all 1080p and 1080i video modes supported 
> by DeckLink.  I would suspect that is not the desired behavior.  See the 
> relevant comments from my review of the first patch.

It is in this case.

>
>> +                    for (i = 8; i < 584; i++) {
>> +                        uint8_t *buf;
>> +                        if (vanc->GetBufferForVerticalBlankingLine(i, 
> (void**)&buf) == S_OK)
>> +                            txt_buf = 
> teletext_data_unit_from_vanc_data(buf, txt_buf, ctx->teletext_lines);
>> +                        if (i == 20)
>> +                            i = 569;
>> +                        if (txt_buf - txt_buf0 > 1611) {
>
> Since you added 1920 bytes to support OP47, it would seem that all 1920 
> bytes may be relevant.  So, this above check may be wrong.

No, I check here that I still have 1920 bytes available in the buffer.

>
>> +                            av_log(avctx, AV_LOG_ERROR, "Too many OP47 
> teletext packets.\n");
>> +                            break;
>> +                        }
>> +                    }
>> +                }
>>                   vanc->Release();

I will send a v2 for this as well.

Thanks,
Marton