[FFmpeg-devel,1/4] avdevice/decklink_dec: Added VANC search for all resolutions

From: Karthick J <kjeyapal@akamai.com>

In preparation to make VANC decode modular, to support multiple other VANC data.

Signed-off-by: Karthick J <kjeyapal@akamai.com>
---
 libavdevice/decklink_dec.cpp | 86 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 76 insertions(+), 10 deletions(-)

On Thu, 31 Aug 2017, kjeyapal@akamai.com wrote:

> From: Karthick J <kjeyapal@akamai.com>
>
> In preparation to make VANC decode modular, to support multiple other VANC data.
>
> Signed-off-by: Karthick J <kjeyapal@akamai.com>
> ---
> libavdevice/decklink_dec.cpp | 86 ++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 76 insertions(+), 10 deletions(-)
>
> diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
> index c271ff3..40ef655 100644
> --- a/libavdevice/decklink_dec.cpp
> +++ b/libavdevice/decklink_dec.cpp
> @@ -46,6 +46,66 @@ extern "C" {
> #include "decklink_common.h"
> #include "decklink_dec.h"
> 
> +/* These VANC line numbers need not be very accurate. In any case
> + * GetBufferForVerticalBlankingLine() will return an error when invalid
> + * ancillary line number was requested. We just need to make sure that the
> + * entire VANC region is covered */

That may well be true, but SMPTE 334M recommands to use lines "from 
the second line after the line specified for switching to the last line 
preceding active video, inclusive", so I'd skip at least the lines up to 
the switching line, in order not to decode stuff which in fact belongs to 
another source... And since we already keeping track of VANC line 
numbers for each mode, it's almost no extra code to do this properly.

> +static int get_vanc_lines_progressive (int height)
> +{
> +    switch (height) {
> +    case 486:
> +        return 39;
> +    case 576:
> +        return 45;
> +    case 720:
> +        return 30;
> +    case 1080:
> +        return 45;
> +    default:
> +        return 0;
> +    }
> +}
> +
> +static void get_vanc_lines_interlaced (int height, int *field0_vanc_end,
> +                                       int *field1_vanc_start, int *field1_vanc_end)
> +{
> +    switch (height) {
> +    // NTSC
> +    case 486:
> +        *field0_vanc_end = 19;
> +        *field1_vanc_start = *field0_vanc_end + 243;
> +        *field1_vanc_end = *field1_vanc_start + 20;
> +        break;
> +    // PAL
> +    case 576:
> +        *field0_vanc_end = 22;
> +        *field1_vanc_start = *field0_vanc_end + 288 + 2;
> +        *field1_vanc_end = *field1_vanc_start + 23;
> +        break;
> +    // 1080i
> +    case 1080:
> +        *field0_vanc_end = 20;
> +        *field1_vanc_start = *field0_vanc_end + 540 + 2;
> +        *field1_vanc_end = *field1_vanc_start + 21;
> +        break;
> +    default:
> +        *field0_vanc_end = *field1_vanc_start = *field1_vanc_end = 0;
> +    }
> +}
> +
> +static void get_vanc_lines(int bmd_field_dominance, int height, int *field0_vanc_end,

you can use BMDFieldDominance type for the first argument.

> +                           int *field1_vanc_start, int *vanc_end)
> +{
> +    if (bmd_field_dominance == bmdLowerFieldFirst || bmd_field_dominance == bmdUpperFieldFirst) {
> +        get_vanc_lines_interlaced (height, field0_vanc_end, field1_vanc_start, vanc_end);
> +    } else if (bmd_field_dominance == bmdProgressiveFrame) {
> +        *vanc_end = get_vanc_lines_progressive(height);
> +        *field0_vanc_end = *field1_vanc_start = -1;
> +    } else {
> +        *field0_vanc_end = *field1_vanc_start = *vanc_end = 0;
> +    }
> +}

Instead of all the getter functions, I think it is simpler to create 
struct for a mode/field combination, and use an array to store the mode 
line settings. Then you can loop through the array and find your settings.

> +
> static uint8_t calc_parity_and_line_offset(int line)
> {
>     uint8_t ret = (line < 313) << 5;
> @@ -502,18 +562,24 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                     }
>                 }
> #endif
> -                if (videoFrame->GetWidth() == 1920 && vanc_format == bmdFormat10BitYUV) {
> -                    int first_active_line = ctx->bmd_field_dominance == bmdProgressiveFrame ? 42 : 584;
> -                    for (i = 8; i < first_active_line; i++) {
> +                if (vanc_format == bmdFormat10BitYUV) {
> +                    int bmd_field_dominance, height;
> +                    int field0_vanc_end, field1_vanc_start, vanc_end;
> +                    get_vanc_lines(ctx->bmd_field_dominance, videoFrame->GetHeight(),
> +                                   &field0_vanc_end, &field1_vanc_start, &vanc_end);
> +                    for (i = 0; i <= vanc_end; 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 (ctx->bmd_field_dominance != bmdProgressiveFrame && i == 20)     // skip field1 active lines
> -                            i = 569;
> -                        if (txt_buf - txt_buf0 > 1611) {   // ensure we still have at least 1920 bytes free in the buffer
> -                            av_log(avctx, AV_LOG_ERROR, "Too many OP47 teletext packets.\n");
> -                            break;
> +                        if (vanc->GetBufferForVerticalBlankingLine(i, (void**)&buf) == S_OK) {
> +                            if (videoFrame->GetWidth() == 1920) {
> +                                txt_buf = teletext_data_unit_from_vanc_data(buf, txt_buf, ctx->teletext_lines);
> +                                if (txt_buf - txt_buf0 > 1611) {   // ensure we still have at least 1920 bytes free in the buffer
> +                                    av_log(avctx, AV_LOG_ERROR, "Too many OP47 teletext packets.\n");
> +                                    break;
> +                                }
> +                            }
>                         }
> +                        if (i == field0_vanc_end)
> +                            i = field1_vanc_start;
>                     }
>                 }
>                 vanc->Release();

Regards,
Marton

>On 9/3/17, 4:48 PM, "Marton Balint" <cus@passwd.hu<mailto:cus@passwd.hu>> wrote:


>That may well be true, but SMPTE 334M recommands to use lines "from

>the second line after the line specified for switching to the last line

>preceding active video, inclusive", so I'd skip at least the lines up to

>the switching line, in order not to decode stuff which in fact belongs to

>another source... And since we already keeping track of VANC line

>numbers for each mode, it's almost no extra code to do this properly.


>Instead of all the getter functions, I think it is simpler to create

>struct for a mode/field combination, and use an array to store the mode

>line settings. Then you can loop through the array and find your settings.


Hi Marton,

Thanks for your comments. Please find the patch attached, incorporating both the above suggestions.

Regards,
Karthick

Overlooked a “signed vs unsigned comparison” compilation warning in the previous patch. Sorry about that. Please use this updated patch(attached) for review.

Regards,
Karthick

P.S : I had sent the same reply sometime back, but a new thread got created. Please ignore that. I am using Outlook on Mac as my mail client and it doesn’t really work along with mailing lists, with extra corporate-level securities in our mail servers. Hope this reply doesn’t create a new thread again :)

On Tue, 5 Sep 2017, Jeyapal, Karthick wrote:

>
> Overlooked a “signed vs unsigned comparison” compilation warning in the previous patch. Sorry about that. Please use this updated patch(attached) for review.

Thanks, I think it is better if we parse all lines after the line 
affected by switching, so I am suggesting these start lines:

480i: 11.. , 274..
480p: 11..
576i: 7.., 320..
576p: 7..
720p: 8..
1080i: 8.., 570..
1080p: 8..

Also there is a condition in the code:

+                        if (i == vanc_line_numbers[idx].field0_vanc_end)
+                            i = vanc_line_numbers[idx].field1_vanc_start;

I think 'i' should be field1_vanc_start - 1, because the loop will 
increase the value of i.

Otherwise the patch looks good.

Thanks,
Marton

Thanks for your comments. Please find the patch attached as per your comments.

Regards,
Karthick