diff mbox

[FFmpeg-devel,3/4] avdevice/decklink_dec: Added Closed caption decode from VANC

Message ID 1504168610-2427-3-git-send-email-kjeyapal@akamai.com
State New
Headers show

Commit Message

Jeyapal, Karthick Aug. 31, 2017, 8:36 a.m. UTC 
From: Karthick J <kjeyapal@akamai.com>

Signed-off-by: Karthick J <kjeyapal@akamai.com>
---
 libavcodec/avcodec.h         |   7 ++
 libavdevice/decklink_dec.cpp | 167 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 165 insertions(+), 9 deletions(-)

Comments

Marton Balint Sept. 3, 2017, 2:49 p.m. UTC | #1 
On Thu, 31 Aug 2017, kjeyapal@akamai.com wrote:

> From: Karthick J <kjeyapal@akamai.com>
>
> Signed-off-by: Karthick J <kjeyapal@akamai.com>
> ---
> libavcodec/avcodec.h         |   7 ++
> libavdevice/decklink_dec.cpp | 167 ++++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 165 insertions(+), 9 deletions(-)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 513236a..d4a458f 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1600,6 +1600,13 @@ enum AVPacketSideDataType {
>     AV_PKT_DATA_CONTENT_LIGHT_LEVEL,
>
>     /**
> +     * ATSC A53 Part 4 Closed Captions. This metadata should be associated with
> +     * a video stream. A53 CC bitstream is stored as uint8_t in AVPacketSideData.data.
> +     * The number of bytes of CC data is AVPacketSideData.size.
> +     */
> +    AV_PKT_DATA_A53_CC,
> +
> +    /**
>      * The number of side data elements (in fact a bit more than it).
>      * This is not part of the public API/ABI in the sense that it may
>      * change when new side data types are added.
> diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
> index 0b88fc8..23e1b35 100644
> --- a/libavdevice/decklink_dec.cpp
> +++ b/libavdevice/decklink_dec.cpp
> @@ -106,6 +106,13 @@ static void get_vanc_lines(int bmd_field_dominance, int height, int *field0_vanc
>     }
> }
> 
> +static inline unsigned parity (unsigned x)

Why don't use you simply use uint16_t here as well? I think we typically 
try to avoid unsigned as a type.

> +{
> +    for (unsigned i = 4 * sizeof (x); i > 0; i /= 2)
> +        x ^= x >> i;
> +    return x & 1;
> +}
> +
> /* The 10-bit VANC data is packed in V210, we only need the luma component. */
> static void extract_luma_from_v210(uint16_t *dst, const uint8_t *src, int width)
> {
> @@ -249,6 +256,152 @@ static uint8_t* teletext_data_unit_from_vanc_data(uint16_t *py, uint8_t *tgt, in
>     return tgt;
> }
> 
> +uint8_t *vanc_to_cc(AVFormatContext *avctx, uint16_t *buf, size_t words,
> +    unsigned &cc_count)
> +{
> +    size_t len = (buf[5] & 0xff) + 6 + 1;
> +
> +    /* CDP follows */
> +    uint16_t *cdp = &buf[6];
> +    if (cdp[0] != 0x96 || cdp[1] != 0x69) {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid CDP header 0x%.2x 0x%.2x", cdp[0], cdp[1]);
> +        return NULL;
> +    }
> +
> +    len -= 7; // remove VANC header and checksum
> +
> +    if (cdp[2] != len) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP len %d != %zu", cdp[2], len);
> +        return NULL;
> +    }
> +
> +    uint8_t cdp_sum = 0;
> +    for (size_t i = 0; i < len - 1; i++)
> +        cdp_sum += cdp[i];
> +    cdp_sum = cdp_sum ? 256 - cdp_sum : 0;
> +    if (cdp[len - 1] != cdp_sum) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP checksum invalid 0x%.4x != 0x%.4x", cdp_sum, cdp[len-1]);
> +        return NULL;
> +    }
> +
> +    uint8_t rate = cdp[3];
> +    if (!(rate & 0x0f)) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP frame rate invalid (0x%.2x)", rate);
> +        return NULL;
> +    }
> +    rate >>= 4;
> +    if (rate > 8) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP frame rate invalid (0x%.2x)", rate);
> +        return NULL;
> +    }
> +
> +    if (!(cdp[4] & 0x43)) /* ccdata_present | caption_service_active | reserved */ {
> +        av_log(avctx, AV_LOG_WARNING, "CDP flags invalid (0x%.2x)", cdp[4]);
> +        return NULL;
> +    }
> +
> +    uint16_t hdr = (cdp[5] << 8) | cdp[6];
> +    if (cdp[7] != 0x72) /* ccdata_id */ {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid ccdata_id 0x%.2x", cdp[7]);
> +        return NULL;
> +    }
> +
> +    cc_count = cdp[8];
> +    if (!(cc_count & 0xe0)) {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid cc_count 0x%.2x", cc_count);
> +        return NULL;
> +    }
> +
> +    cc_count &= 0x1f;
> +    if ((len - 13) < cc_count * 3) {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid cc_count %d (> %zu)", cc_count * 3, len - 13);
> +        return NULL;
> +    }
> +
> +    if (cdp[len - 4] != 0x74) /* footer id */ {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid footer id 0x%.2x", cdp[len-4]);
> +        return NULL;
> +    }
> +
> +    uint16_t ftr = (cdp[len - 3] << 8) | cdp[len - 2];
> +    if (ftr != hdr) {
> +        av_log(avctx, AV_LOG_WARNING, "Header 0x%.4x != Footer 0x%.4x", hdr, ftr);
> +        return NULL;
> +    }
> +
> +    uint8_t *cc = (uint8_t *)av_malloc(cc_count * 3);
> +
> +    for (size_t i = 0; i < cc_count; i++) {
> +        cc[3*i + 0] = cdp[9 + 3*i+0] /* & 3 */;
> +        cc[3*i + 1] = cdp[9 + 3*i+1];
> +        cc[3*i + 2] = cdp[9 + 3*i+2];
> +    }
> +
> +    cc_count *= 3;
> +    return cc;
> +}
> +
> +uint8_t *get_metadata(AVFormatContext *avctx, uint16_t *buf, size_t width,
> +                      uint8_t *tgt, size_t tgt_size, AVPacket *pkt)
> +{
> +    decklink_cctx *cctx = (struct decklink_cctx *) avctx->priv_data;
> +    uint16_t did = buf[3] & 0xFF;                                  // data id
> +    uint16_t sdid = buf[4] & 0xFF;                                 // secondary data id
> +
> +    static const uint8_t vanc_header[6] = { 0x00, 0x00, 0xff, 0x03, 0xff, 0x03 };
> +    if (memcmp(vanc_header, buf, 3 * 2)) {
> +        /* Does not start with the VANC header */
> +        return tgt;
> +    }
> +
> +    size_t len = (buf[5] & 0xff) + 6 + 1;
> +    if (len > width) {
> +        av_log(avctx, AV_LOG_WARNING, "Data Count (%zu) > line length (%zu)\n",
> +                len, width);
> +        return tgt;
> +    }
> +
> +    uint16_t vanc_sum = 0;
> +    for (size_t i = 3; i < len - 1; i++) {
> +        uint16_t v = buf[i];
> +        int np = v >> 8;
> +        int p = parity(v & 0xff);
> +        if ((!!p ^ !!(v & 0x100)) || (np != 1 && np != 2)) {
> +            av_log(avctx, AV_LOG_WARNING, "Parity incorrect for word %zu\n", i);
> +            return tgt;
> +        }
> +        vanc_sum += v;
> +        vanc_sum &= 0x1ff;
> +        buf[i] &= 0xff;
> +    }
> +
> +    vanc_sum |= ((~vanc_sum & 0x100) << 1);
> +    if (buf[len - 1] != vanc_sum) {
> +        av_log(avctx, AV_LOG_WARNING,
> +                "VANC checksum incorrect: 0x%.4x != 0x%.4x\n", vanc_sum,
> +                buf[len - 1]);
> +        return tgt;
> +    }
> +
> +    if (did == 0x43 && (sdid == 0x02 || sdid == 0x03) && cctx->teletext_lines &&
> +        width == 1920 && tgt_size >= 1920) {
> +        tgt = teletext_data_unit_from_vanc_data(buf, tgt, cctx->teletext_lines);
> +    } else if (did == 0x61 && sdid == 0x01) {
> +        unsigned int data_len;
> +        uint8_t *data = vanc_to_cc(avctx, buf, width, data_len);
> +        if (data) {
> +            uint8_t *pkt_cc = av_packet_new_side_data(pkt, AV_PKT_DATA_A53_CC, data_len);
> +            memcpy(pkt_cc, data, data_len);
> +            av_free(data);
> +        }
> +    } else {
> +        av_log(avctx, AV_LOG_WARNING, "Unknown meta data DID = 0x%.2x SDID = 0x%.2x\n",
> +                did, sdid);
> +    }

This VANC parser does not seem right. VANC header can appear multiple 
times, you seem to parse only the first DID. Also, you should simply 
ignore unknown DID-s, ffmpeg will not be able to parse all types there 
are, there is no need to warn.

I suggest you base your code on the existing VANC parser I used for OP47, 
it can also handle VANC packets encapsulated in a VANC multipacket. Add 
your DID/SDID-s and your parity checker code to the 
teletext_data_unit_from_ancillary_packet function. (Obviously you can 
rename the function to better reflect the new generic purpose). Maybe you 
can split this patch even further: rename functions / add parity checker / 
add A53CC.

> +
> +    return tgt;
> +}
> +
> static void avpacket_queue_init(AVFormatContext *avctx, AVPacketQueue *q)
> {
>     struct decklink_cctx *ctx = (struct decklink_cctx *)avctx->priv_data;
> @@ -537,7 +690,7 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                            videoFrame->GetHeight();
>         //fprintf(stderr,"Video Frame size %d ts %d\n", pkt.size, pkt.pts);
> 
> -        if (!no_video && ctx->teletext_lines) {
> +        if (!no_video) {
>             IDeckLinkVideoFrameAncillary *vanc;
>             AVPacket txt_pkt;
>             uint8_t txt_buf0[3531]; // 35 * 46 bytes decoded teletext lines + 1 byte data_identifier + 1920 bytes OP47 decode buffer
> @@ -550,7 +703,8 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                 txt_buf[0] = 0x10;    // data_identifier - EBU_data
>                 txt_buf++;
> #if CONFIG_LIBZVBI
> -                if (ctx->bmd_mode == bmdModePAL && (vanc_format == bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) {
> +                if (ctx->bmd_mode == bmdModePAL && ctx->teletext_lines &&
> +                    (vanc_format == bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) {
>                     av_assert0(videoFrame->GetWidth() == 720);
>                     for (i = 6; i < 336; i++, line_mask <<= 1) {
>                         uint8_t *buf;
> @@ -575,13 +729,8 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                         if (vanc->GetBufferForVerticalBlankingLine(i, (void**)&buf) == S_OK) {
>                             uint16_t luma_vanc[4096]; // Allocated for highest width (4K)
>                             extract_luma_from_v210(luma_vanc, buf, videoFrame->GetWidth());
> -                            if (videoFrame->GetWidth() == 1920) {
> -                                txt_buf = teletext_data_unit_from_vanc_data(luma_vanc, 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;
> -                                }
> -                            }
> +                            txt_buf = get_metadata(avctx, luma_vanc, videoFrame->GetWidth(),
> +                                                   txt_buf, 3531 - (txt_buf - txt_buf0), &pkt);
>                         }
>                         if (i == field0_vanc_end)
>                             i = field1_vanc_start;
> -- 
> 1.9.1
>

Regards,
Marton
Jeyapal, Karthick Sept. 5, 2017, 11:27 a.m. UTC | #2 
>On 9/3/17, 8:19 PM, "Marton Balint" <cus@passwd.hu<mailto:cus@passwd.hu>> wrote:


>Why don't use you simply use uint16_t here as well? I think we typically

>try to avoid unsigned as a type.

Done. Changed it to uint16_t.

>This VANC parser does not seem right. VANC header can appear multiple

>times, you seem to parse only the first DID. Also, you should simply

>ignore unknown DID-s, ffmpeg will not be able to parse all types there

>are, there is no need to warn.

Thanks for pointing it out. Have handled it now. Also changed the warning log to debug log.

Please find the patch attached.

Regards,
Marton
Marton Balint Sept. 7, 2017, 7:31 p.m. UTC | #3 
On Tue, 5 Sep 2017, Jeyapal, Karthick wrote:

>
>
>> On 9/3/17, 8:19 PM, "Marton Balint" <cus@passwd.hu<mailto:cus@passwd.hu>> wrote:
>
>> Why don't use you simply use uint16_t here as well? I think we typically
>> try to avoid unsigned as a type.
> Done. Changed it to uint16_t.
>
>> This VANC parser does not seem right. VANC header can appear multiple
>> times, you seem to parse only the first DID. Also, you should simply
>> ignore unknown DID-s, ffmpeg will not be able to parse all types there
>> are, there is no need to warn.
> Thanks for pointing it out. Have handled it now. Also changed the warning log to debug log.
>
> Please find the patch attached.

Here are some more comments for the code:

[..]

> -static uint8_t* teletext_data_unit_from_vanc_data(uint16_t *py, uint8_t *tgt, int64_t wanted_lines)
> +uint8_t *vanc_to_cc(AVFormatContext *avctx, uint16_t *buf, size_t words,
> +    unsigned &cc_count)
>  {
> -    uint16_t *pend = py + 1920;
> +    size_t len = (buf[5] & 0xff) + 6 + 1;
> 
> -    while (py < pend - 6) {
> -        if (py[0] == 0 && py[1] == 0x3ff && py[2] == 0x3ff) {           // ancillary data flag
> -            py += 3;
> -            tgt = teletext_data_unit_from_ancillary_packet(py, pend, tgt, wanted_lines, 0);
> -            py += py[2] & 255;
> +    /* CDP follows */
> +    uint16_t *cdp = &buf[6];

Inline declarations are not allowed in ffmpeg.

> +    if (cdp[0] != 0x96 || cdp[1] != 0x69) {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid CDP header 0x%.2x 0x%.2x", cdp[0], cdp[1]);
> +        return NULL;
> +    }
> +
> +    len -= 7; // remove VANC header and checksum
> +
> +    if (cdp[2] != len) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP len %d != %zu", cdp[2], len);
> +        return NULL;
> +    }
> +
> +    uint8_t cdp_sum = 0;

inline declaration

> +    for (size_t i = 0; i < len - 1; i++)

inline declaration

> +        cdp_sum += cdp[i];
> +    cdp_sum = cdp_sum ? 256 - cdp_sum : 0;
> +    if (cdp[len - 1] != cdp_sum) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP checksum invalid 0x%.4x != 0x%.4x", cdp_sum, cdp[len-1]);
> +        return NULL;
> +    }
> +
> +    uint8_t rate = cdp[3];

inline declaration

> +    if (!(rate & 0x0f)) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP frame rate invalid (0x%.2x)", rate);
> +        return NULL;
> +    }
> +    rate >>= 4;
> +    if (rate > 8) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP frame rate invalid (0x%.2x)", rate);
> +        return NULL;
> +    }
> +
> +    if (!(cdp[4] & 0x43)) /* ccdata_present | caption_service_active | reserved */ {
> +        av_log(avctx, AV_LOG_WARNING, "CDP flags invalid (0x%.2x)", cdp[4]);
> +        return NULL;
> +    }
> +
> +    uint16_t hdr = (cdp[5] << 8) | cdp[6];

inline declaration

> +    if (cdp[7] != 0x72) /* ccdata_id */ {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid ccdata_id 0x%.2x", cdp[7]);
> +        return NULL;
> +    }
> +
> +    cc_count = cdp[8];
> +    if (!(cc_count & 0xe0)) {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid cc_count 0x%.2x", cc_count);
> +        return NULL;
> +    }
> +
> +    cc_count &= 0x1f;
> +    if ((len - 13) < cc_count * 3) {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid cc_count %d (> %zu)", cc_count * 3, len - 13);
> +        return NULL;
> +    }
> +
> +    if (cdp[len - 4] != 0x74) /* footer id */ {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid footer id 0x%.2x", cdp[len-4]);
> +        return NULL;
> +    }
> +
> +    uint16_t ftr = (cdp[len - 3] << 8) | cdp[len - 2];

inline declaration

> +    if (ftr != hdr) {
> +        av_log(avctx, AV_LOG_WARNING, "Header 0x%.4x != Footer 0x%.4x", hdr, ftr);
> +        return NULL;
> +    }
> +
> +    uint8_t *cc = (uint8_t *)av_malloc(cc_count * 3);

inline declaration
malloc missing error check

> +
> +    for (size_t i = 0; i < cc_count; i++) {

inline declaration

> +        cc[3*i + 0] = cdp[9 + 3*i+0] /* & 3 */;
> +        cc[3*i + 1] = cdp[9 + 3*i+1];
> +        cc[3*i + 2] = cdp[9 + 3*i+2];
> +    }
> +
> +    cc_count *= 3;
> +    return cc;
> +}
> +
> +uint8_t *get_metadata(AVFormatContext *avctx, uint16_t *buf, size_t width,
> +                      uint8_t *tgt, size_t tgt_size, AVPacket *pkt)
> +{
> +    decklink_cctx *cctx = (struct decklink_cctx *) avctx->priv_data;
> +    uint16_t *max_buf = buf + width;
> +
> +    static const uint8_t vanc_header[6] = { 0x00, 0x00, 0xff, 0x03, 0xff, 0x03 };
> +    while (buf < max_buf - 6) {
> +        uint16_t did = buf[3] & 0xFF;                                  // data id
> +        uint16_t sdid = buf[4] & 0xFF;                                 // secondary data id
> +        /* Check for VANC header */
> +        if (memcmp(vanc_header, buf, 3 * 2)) {

Maybe it's personal preference, but I find the (buf[0] == 0 && buf[1] ==
0x3ff && buf[2] == 0x3ff) better. Also, maybe performance wise it might be faster
then calling memcmp each time, although I am not sure.

Also since we are going to parse vanc in every capture, I wonder if we 
should simply take into account the general requirement that vanc packets 
should start immediately and they should be contiguous. As far as I know, 
with compliant sources, we can simply return here instead of continuing 
the loop, and that definitely improves performance.

> +            buf++;
> +            continue;
> +        }
> +
> +        size_t len = (buf[5] & 0xff) + 6 + 1;

inline declaration

> +        if (len > max_buf - buf) {
> +            av_log(avctx, AV_LOG_WARNING, "Data Count (%zu) > data left (%zu)\n",
> +                    len, max_buf - buf);
> +            return tgt;
> +        }
> +
> +        uint16_t vanc_sum = 0;

inline declaration

> +        for (size_t i = 3; i < len - 1; i++) {

inline declaration

> +            uint16_t v = buf[i];
> +            int np = v >> 8;
> +            int p = parity(v & 0xff);
> +            if ((!!p ^ !!(v & 0x100)) || (np != 1 && np != 2)) {
> +                av_log(avctx, AV_LOG_WARNING, "Parity incorrect for word %zu\n", i);

In case of a parity error, you should only skip the current VANC packet, and continue parsing

> +                return tgt;
> +            }
> +            vanc_sum += v;
> +            vanc_sum &= 0x1ff;
> +            buf[i] &= 0xff;
> +        }
> +
> +        vanc_sum |= ((~vanc_sum & 0x100) << 1);
> +        if (buf[len - 1] != vanc_sum) {
> +            av_log(avctx, AV_LOG_WARNING,
> +                    "VANC checksum incorrect: 0x%.4x != 0x%.4x\n", vanc_sum,
> +                    buf[len - 1]);

You can also continue with the next VANC packet here instead of returning

> +            return tgt;
> +        }
> +
> +        if (did == 0x43 && (sdid == 0x02 || sdid == 0x03) && cctx->teletext_lines &&
> +            width == 1920 && tgt_size >= 1920) {
> +            tgt = teletext_data_unit_from_ancillary_packet(buf, buf + 1920, tgt, cctx->teletext_lines, 0);

this should be
                tgt = teletext_data_unit_from_ancillary_packet(buf, buf + len, tgt, cctx->teletext_lines, 1);

> +        } else if (did == 0x61 && sdid == 0x01) {
> +            unsigned int data_len;
> +            uint8_t *data = vanc_to_cc(avctx, buf, width, data_len);
> +            if (data) {
> +                uint8_t *pkt_cc = av_packet_new_side_data(pkt, AV_PKT_DATA_A53_CC, data_len);

missing error check

> +                memcpy(pkt_cc, data, data_len);
> +                av_free(data);
> +            }
>          } else {
> -            py++;
> +            av_log(avctx, AV_LOG_DEBUG, "Unknown meta data DID = 0x%.2x SDID = 0x%.2x\n",
> +                    did, sdid);
>          }
> +        buf += len;
>      }
> +
>      return tgt;
>  }
> 
> @@ -536,7 +678,7 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                             videoFrame->GetHeight();
>          //fprintf(stderr,"Video Frame size %d ts %d\n", pkt.size, pkt.pts);
> 
> -        if (!no_video && ctx->teletext_lines) {
> +        if (!no_video) {
>              IDeckLinkVideoFrameAncillary *vanc;
>              AVPacket txt_pkt;
>              uint8_t txt_buf0[3531]; // 35 * 46 bytes decoded teletext lines + 1 byte data_identifier + 1920 bytes OP47 decode buffer
> @@ -549,7 +691,8 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                  txt_buf[0] = 0x10;    // data_identifier - EBU_data
>                  txt_buf++;
>  #if CONFIG_LIBZVBI
> -                if (ctx->bmd_mode == bmdModePAL && (vanc_format == bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) {
> +                if (ctx->bmd_mode == bmdModePAL && ctx->teletext_lines &&
> +                    (vanc_format == bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) {
>                      av_assert0(videoFrame->GetWidth() == 720);
>                      for (i = 6; i < 336; i++, line_mask <<= 1) {
>                          uint8_t *buf;
> @@ -571,13 +714,8 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                          if (vanc->GetBufferForVerticalBlankingLine(i, (void**)&buf) == S_OK) {
>                              uint16_t luma_vanc[MAX_WIDTH_VANC];
>                              extract_luma_from_v210(luma_vanc, buf, videoFrame->GetWidth());
> -                            if (videoFrame->GetWidth() == 1920) {
> -                                txt_buf = teletext_data_unit_from_vanc_data(luma_vanc, 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;
> -                                }
> -                            }
> +                            txt_buf = get_metadata(avctx, luma_vanc, videoFrame->GetWidth(),
> +                                                   txt_buf, 3531 - (txt_buf - txt_buf0), &pkt);

sizeof(txt_buf0) instead of 3531

>                          }
>                          if (i == vanc_line_numbers[idx].field0_vanc_end)
>                              i = vanc_line_numbers[idx].field1_vanc_start;
> -- 
> 1.9.1
>

Regards,
Marton
Jeyapal, Karthick Sept. 14, 2017, 5:45 a.m. UTC | #4 
Hi Marton,

Thanks a lot for your comments. I have modified the patch as per all your comments, except the below one.

>> +            tgt = teletext_data_unit_from_ancillary_packet(buf, buf + 1920, tgt, cctx->teletext_lines, 0);

>

>this should be

>                tgt = teletext_data_unit_from_ancillary_packet(buf, buf + len, tgt, cctx->teletext_lines, 1);

>


I have modified it as

tgt = teletext_data_unit_from_ancillary_packet(buf, buf + len, tgt, cctx->teletext_lines, 0);

That was just to be in consistency with the existing code. Because current code passes allow_multipacket as 0. Let me know if my understanding here is correct.

Please find the patch attached.

Regards,
Karthick
Marton Balint Sept. 14, 2017, 10:17 p.m. UTC | #5 
On Thu, 14 Sep 2017, Jeyapal, Karthick wrote:

> Hi Marton,
>
> Thanks a lot for your comments. I have modified the patch as per all your comments, except the below one.
>
>>> +            tgt = teletext_data_unit_from_ancillary_packet(buf, buf + 1920, tgt, cctx->teletext_lines, 0);
>>
>> this should be
>>                tgt = teletext_data_unit_from_ancillary_packet(buf, buf + len, tgt, cctx->teletext_lines, 1);
>>
>
> I have modified it as
>
> tgt = teletext_data_unit_from_ancillary_packet(buf, buf + len, tgt, cctx->teletext_lines, 0);
>
> That was just to be in consistency with the existing code. Because current code passes allow_multipacket as 0. Let me know if my understanding here is correct.

Shoot, then the original code was incorrect :).

I will take a final look at your patches next week.

Regards,
Marton
Jeyapal, Karthick Sept. 15, 2017, 9:29 a.m. UTC | #6 
>Shoot, then the original code was incorrect :).

>

>I will take a final look at your patches next week.

>

>Regards,

>Marton


Thanks. And I forgot to mention a thing. Some parts of that code in this patch was started from VLC’s source code. Source link here https://fossies.org/linux/vlc/modules/access/sdi.c
That code is also in LGPL license, hence I think it is fine to copy it. But I am just giving a disclaimer, so that neither ffmpeg nor the company I represent gets into any legal trouble because of that.
I am not a legal expert here, but do you think I should also add those copyright lines from sdi.c in this patch as well.

Regards,
Karthick
Carl Eugen Hoyos Sept. 19, 2017, 11:57 p.m. UTC | #7 
2017-09-15 11:29 GMT+02:00 Jeyapal, Karthick <kjeyapal@akamai.com>:
> And I forgot to mention a thing. Some parts of that code in this patch
> was started from VLC’s source code. Source link here
> https://fossies.org/linux/vlc/modules/access/sdi.c

Then please add the relevant copyright line to the file you changed.
(Probably "Rafaël Carré" but please verify yourself)

Carl Eugen
Jeyapal, Karthick Sept. 20, 2017, 6:16 a.m. UTC | #8 
>Then please add the relevant copyright line to the file you changed.

>(Probably "Rafaël Carré" but please verify yourself)

>

>Carl Eugen


Thanks for the clarification. I have added the copyright line as you have advised. (Yes, I have verified that that “Rafaël Carré” was the original author of that code).

Regards,
Karthick
Marton Balint Sept. 25, 2017, 6:36 p.m. UTC | #9 
On Wed, 20 Sep 2017, Jeyapal, Karthick wrote:

>> Then please add the relevant copyright line to the file you changed.
>> (Probably "Rafaël Carré" but please verify yourself)
>>
>> Carl Eugen
>
> Thanks for the clarification. I have added the copyright line as you 
> have advised. (Yes, I have verified that that “Rafaël Carré” was the 
> original author of that code).

Sorry for the late reply, the last was busy week. Here are some more 
comments:

> From: Karthick J <kjeyapal@akamai.com>
> Date: Wed, 30 Aug 2017 11:43:16 +0530
> Subject: [PATCH 3/4] avdevice/decklink_dec: Added Closed caption decode from
>  VANC
> 
> Signed-off-by: Karthick J <kjeyapal@akamai.com>
> ---
>  libavcodec/avcodec.h         |   7 ++
>  libavdevice/decklink_dec.cpp | 180 +++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 170 insertions(+), 17 deletions(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 513236a..d4a458f 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1600,6 +1600,13 @@ enum AVPacketSideDataType {
>      AV_PKT_DATA_CONTENT_LIGHT_LEVEL,
>
>      /**
> +     * ATSC A53 Part 4 Closed Captions. This metadata should be associated with
> +     * a video stream. A53 CC bitstream is stored as uint8_t in AVPacketSideData.data.
> +     * The number of bytes of CC data is AVPacketSideData.size.
> +     */
> +    AV_PKT_DATA_A53_CC,
> +

This needs a libavcodec/version.h micro bump.

Also add a descriptive name for this in libavcodec/avpacket.c / av_packet_side_data_name, e.g:

case AV_PKT_DATA_A53_CC:                     return "A53 Closed Captions";

> +    /**
>       * The number of side data elements (in fact a bit more than it).
>       * This is not part of the public API/ABI in the sense that it may
>       * change when new side data types are added.
> diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
> index 38e6bd8..7ccbaae 100644
> --- a/libavdevice/decklink_dec.cpp
> +++ b/libavdevice/decklink_dec.cpp
> @@ -1,6 +1,7 @@
>  /*
>   * Blackmagic DeckLink input
>   * Copyright (c) 2013-2014 Luca Barbato, Deti Fliegl
> + * Copyright (c) 2014 Rafaël Carré
>   * Copyright (c) 2017 Akamai Technologies, Inc.
>   *
>   * This file is part of FFmpeg.
> @@ -105,6 +106,14 @@ static int get_vanc_line_idx(BMDDisplayMode mode)
>      return i - 1;
>  }
> 
> +static inline uint16_t parity (uint16_t x)
> +{
> +    uint16_t i;
> +    for (i = 4 * sizeof (x); i > 0; i /= 2)
> +        x ^= x >> i;
> +    return x & 1;
> +}
> +
>  /* The 10-bit VANC data is packed in V210, we only need the luma component. */
>  static void extract_luma_from_v210(uint16_t *dst, const uint8_t *src, int width)
>  {
> @@ -232,19 +241,160 @@ static uint8_t* teletext_data_unit_from_ancillary_packet(uint16_t *py, uint16_t
>      return tgt;
>  }
> 
> -static uint8_t* teletext_data_unit_from_vanc_data(uint16_t *py, uint8_t *tgt, int64_t wanted_lines)
> +uint8_t *vanc_to_cc(AVFormatContext *avctx, uint16_t *buf, size_t words,
> +    unsigned &cc_count)
>  {
> -    uint16_t *pend = py + 1920;
> +    size_t i, len = (buf[5] & 0xff) + 6 + 1;
> +    uint8_t cdp_sum, rate;
> +    uint16_t hdr, ftr;
> +    uint8_t *cc;
> +    uint16_t *cdp = &buf[6]; // CDP follows
> +    if (cdp[0] != 0x96 || cdp[1] != 0x69) {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid CDP header 0x%.2x 0x%.2x", cdp[0], cdp[1]);
> +        return NULL;
> +    }
> 
> -    while (py < pend - 6) {
> -        if (py[0] == 0 && py[1] == 0x3ff && py[2] == 0x3ff) {           // ancillary data flag
> -            py += 3;
> -            tgt = teletext_data_unit_from_ancillary_packet(py, pend, tgt, wanted_lines, 0);
> -            py += py[2] & 255;
> +    len -= 7; // remove VANC header and checksum
> +
> +    if (cdp[2] != len) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP len %d != %zu", cdp[2], len);
> +        return NULL;
> +    }
> +
> +    cdp_sum = 0;
> +    for (i = 0; i < len - 1; i++)
> +        cdp_sum += cdp[i];
> +    cdp_sum = cdp_sum ? 256 - cdp_sum : 0;
> +    if (cdp[len - 1] != cdp_sum) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP checksum invalid 0x%.4x != 0x%.4x", cdp_sum, cdp[len-1]);
> +        return NULL;
> +    }
> +
> +    rate = cdp[3];
> +    if (!(rate & 0x0f)) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP frame rate invalid (0x%.2x)", rate);
> +        return NULL;
> +    }
> +    rate >>= 4;
> +    if (rate > 8) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP frame rate invalid (0x%.2x)", rate);
> +        return NULL;
> +    }
> +
> +    if (!(cdp[4] & 0x43)) /* ccdata_present | caption_service_active | reserved */ {
> +        av_log(avctx, AV_LOG_WARNING, "CDP flags invalid (0x%.2x)", cdp[4]);
> +        return NULL;
> +    }
> +
> +    hdr = (cdp[5] << 8) | cdp[6];
> +    if (cdp[7] != 0x72) /* ccdata_id */ {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid ccdata_id 0x%.2x", cdp[7]);
> +        return NULL;
> +    }
> +
> +    cc_count = cdp[8];
> +    if (!(cc_count & 0xe0)) {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid cc_count 0x%.2x", cc_count);
> +        return NULL;
> +    }
> +
> +    cc_count &= 0x1f;
> +    if ((len - 13) < cc_count * 3) {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid cc_count %d (> %zu)", cc_count * 3, len - 13);
> +        return NULL;
> +    }
> +
> +    if (cdp[len - 4] != 0x74) /* footer id */ {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid footer id 0x%.2x", cdp[len-4]);
> +        return NULL;
> +    }
> +
> +    ftr = (cdp[len - 3] << 8) | cdp[len - 2];
> +    if (ftr != hdr) {
> +        av_log(avctx, AV_LOG_WARNING, "Header 0x%.4x != Footer 0x%.4x", hdr, ftr);
> +        return NULL;
> +    }
> +
> +    cc = (uint8_t *)av_malloc(cc_count * 3);
> +    if (cc == NULL) {
> +        av_log(avctx, AV_LOG_WARNING, "CC - av_malloc failed for cc_count = %d", cc_count);
> +        return NULL;
> +    }
> +
> +    for (size_t i = 0; i < cc_count; i++) {
> +        cc[3*i + 0] = cdp[9 + 3*i+0] /* & 3 */;
> +        cc[3*i + 1] = cdp[9 + 3*i+1];
> +        cc[3*i + 2] = cdp[9 + 3*i+2];
> +    }
> +
> +    cc_count *= 3;
> +    return cc;
> +}
> +
> +uint8_t *get_metadata(AVFormatContext *avctx, uint16_t *buf, size_t width,
> +                      uint8_t *tgt, size_t tgt_size, AVPacket *pkt)
> +{
> +    decklink_cctx *cctx = (struct decklink_cctx *) avctx->priv_data;
> +    uint16_t *max_buf = buf + width;
> +
> +    while (buf < max_buf - 6) {
> +        int len, i;
> +        uint16_t vanc_sum = 0;
> +        uint16_t did = buf[3] & 0xFF;                                  // data id
> +        uint16_t sdid = buf[4] & 0xFF;                                 // secondary data id
> +        /* Check for VANC header */
> +        if (buf[0] != 0 || buf[1] != 0x3ff || buf[2] != 0x3ff) {
> +            return tgt;
> +        }
> +
> +        len = (buf[5] & 0xff) + 6 + 1;
> +        if (len > max_buf - buf) {
> +            av_log(avctx, AV_LOG_WARNING, "Data Count (%d) > data left (%zu)\n",
> +                    len, max_buf - buf);
> +            return tgt;
> +        }
> +
> +        for (i = 3; i < len - 1; i++) {
> +            uint16_t v = buf[i];
> +            int np = v >> 8;
> +            int p = parity(v & 0xff);
> +            if ((!!p ^ !!(v & 0x100)) || (np != 1 && np != 2)) {
> +                av_log(avctx, AV_LOG_WARNING, "Parity incorrect for word %d\n", i);
> +                goto skip_packet;
> +            }

For some data types user data words may not contain a parity, therefore it
should not be checked in generic parser code, only in data id specific one.

Maybe best to create a separate function for parity checking and call that
after we know the data type? On the other hand parity checking for DID, SDID
and DC can remain in generic code.

> +            vanc_sum += v;
> +            vanc_sum &= 0x1ff;

This may only be needed once after the loop.

> +            buf[i] &= 0xff;

This is also seems data type specific, and also breaks existing teletext op47
code as it is written for 10bit data.

> +        }
> +
> +        vanc_sum |= ((~vanc_sum & 0x100) << 1);
> +        if (buf[len - 1] != vanc_sum) {
> +            av_log(avctx, AV_LOG_WARNING,
> +                    "VANC checksum incorrect: 0x%.4x != 0x%.4x\n", vanc_sum,
> +                    buf[len - 1]);
> +            goto skip_packet;
> +        }
> +
> +        if (did == 0x43 && (sdid == 0x02 || sdid == 0x03) && cctx->teletext_lines &&
> +            width == 1920 && tgt_size >= 1920) {
> +            tgt = teletext_data_unit_from_ancillary_packet(buf, buf + len, tgt, cctx->teletext_lines, 0);

This is actually buf + 3 and not buf, because this function needs a buffer starting with DID.

> +        } else if (did == 0x61 && sdid == 0x01) {
> +            unsigned int data_len;
> +            uint8_t *data = vanc_to_cc(avctx, buf, width, data_len);
> +            if (data) {
> +                uint8_t *pkt_cc = av_packet_new_side_data(pkt, AV_PKT_DATA_A53_CC, data_len);
> +                if (pkt_cc)
> +                    memcpy(pkt_cc, data, data_len);
> +                av_free(data);
> +            }
>          } else {
> -            py++;
> +            av_log(avctx, AV_LOG_DEBUG, "Unknown meta data DID = 0x%.2x SDID = 0x%.2x\n",
> +                    did, sdid);
>          }
> +skip_packet:
> +        buf += len;
>      }
> +
>      return tgt;
>  }
> 
> @@ -536,7 +686,7 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                             videoFrame->GetHeight();
>          //fprintf(stderr,"Video Frame size %d ts %d\n", pkt.size, pkt.pts);
> 
> -        if (!no_video && ctx->teletext_lines) {
> +        if (!no_video) {
>              IDeckLinkVideoFrameAncillary *vanc;
>              AVPacket txt_pkt;
>              uint8_t txt_buf0[3531]; // 35 * 46 bytes decoded teletext lines + 1 byte data_identifier + 1920 bytes OP47 decode buffer
> @@ -549,7 +699,8 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                  txt_buf[0] = 0x10;    // data_identifier - EBU_data
>                  txt_buf++;
>  #if CONFIG_LIBZVBI
> -                if (ctx->bmd_mode == bmdModePAL && (vanc_format == bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) {
> +                if (ctx->bmd_mode == bmdModePAL && ctx->teletext_lines &&
> +                    (vanc_format == bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) {
>                      av_assert0(videoFrame->GetWidth() == 720);
>                      for (i = 6; i < 336; i++, line_mask <<= 1) {
>                          uint8_t *buf;
> @@ -571,13 +722,8 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                          if (vanc->GetBufferForVerticalBlankingLine(i, (void**)&buf) == S_OK) {
>                              uint16_t luma_vanc[MAX_WIDTH_VANC];
>                              extract_luma_from_v210(luma_vanc, buf, videoFrame->GetWidth());
> -                            if (videoFrame->GetWidth() == 1920) {
> -                                txt_buf = teletext_data_unit_from_vanc_data(luma_vanc, 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;
> -                                }
> -                            }
> +                            txt_buf = get_metadata(avctx, luma_vanc, videoFrame->GetWidth(),
> +                                                   txt_buf, sizeof(txt_buf0) - (txt_buf - txt_buf0), &pkt);
>                          }
>                          if (i == vanc_line_numbers[idx].field0_vanc_end)
>                              i = vanc_line_numbers[idx].field1_vanc_start - 1;
> --

Regards,
Marton
Jeyapal, Karthick Sept. 27, 2017, 4:55 a.m. UTC | #10 
>Sorry for the late reply, the last was busy week. Here are some more

>comments:


Thanks for your comments. Please find the updated patch attached with your comments incorporated.

Regards,
Karthick
Marton Balint Sept. 27, 2017, 10:01 p.m. UTC | #11 
On Wed, 27 Sep 2017, Jeyapal, Karthick wrote:

>> Sorry for the late reply, the last was busy week. Here are some more
>> comments:
>
> Thanks for your comments. Please find the updated patch attached with your comments incorporated.

Sorry, still not quite there... Here are some further comments:

> From 9a572cb8b4ea599ee6bc1eedd6c7ca6868cfe0c6 Mon Sep 17 00:00:00 2001
> From: Karthick J <kjeyapal@akamai.com>
> Date: Wed, 30 Aug 2017 11:43:16 +0530
> Subject: [PATCH 3/4] avdevice/decklink_dec: Added Closed caption decode from
>  VANC
> 
> Signed-off-by: Karthick J <kjeyapal@akamai.com>
> ---
>  libavcodec/avcodec.h         |   7 ++
>  libavcodec/avpacket.c        |   1 +
>  libavcodec/version.h         |   2 +-
>  libavdevice/decklink_dec.cpp | 196 +++++++++++++++++++++++++++++++++++++++----
>  4 files changed, 188 insertions(+), 18 deletions(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 513236a..d4a458f 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1600,6 +1600,13 @@ enum AVPacketSideDataType {
>      AV_PKT_DATA_CONTENT_LIGHT_LEVEL,
>
>      /**
> +     * ATSC A53 Part 4 Closed Captions. This metadata should be associated with
> +     * a video stream. A53 CC bitstream is stored as uint8_t in AVPacketSideData.data.
> +     * The number of bytes of CC data is AVPacketSideData.size.
> +     */
> +    AV_PKT_DATA_A53_CC,

I forgot to mention that you should also add a note about this to doc/APIChanges, sorry.

> +
> +    /**
>       * The number of side data elements (in fact a bit more than it).
>       * This is not part of the public API/ABI in the sense that it may
>       * change when new side data types are added.
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index 5ce3228..a292b15 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -387,6 +387,7 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type)
>      case AV_PKT_DATA_MASTERING_DISPLAY_METADATA: return "Mastering display metadata";
>      case AV_PKT_DATA_CONTENT_LIGHT_LEVEL:        return "Content light level metadata";
>      case AV_PKT_DATA_SPHERICAL:                  return "Spherical Mapping";
> +    case AV_PKT_DATA_A53_CC:                     return "A53 Closed Captions";
>      }
>      return NULL;
>  }
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 22cab7b..29cdb85 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -29,7 +29,7 @@
>
>  #define LIBAVCODEC_VERSION_MAJOR  57
>  #define LIBAVCODEC_VERSION_MINOR 104
> -#define LIBAVCODEC_VERSION_MICRO 100
> +#define LIBAVCODEC_VERSION_MICRO 101
>
>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>                                                 LIBAVCODEC_VERSION_MINOR, \
> diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
> index 38e6bd8..a24f563 100644
> --- a/libavdevice/decklink_dec.cpp
> +++ b/libavdevice/decklink_dec.cpp
> @@ -1,6 +1,7 @@
>  /*
>   * Blackmagic DeckLink input
>   * Copyright (c) 2013-2014 Luca Barbato, Deti Fliegl
> + * Copyright (c) 2014 Rafaël Carré
>   * Copyright (c) 2017 Akamai Technologies, Inc.
>   *
>   * This file is part of FFmpeg.
> @@ -105,6 +106,42 @@ static int get_vanc_line_idx(BMDDisplayMode mode)
>      return i - 1;
>  }
> 
> +static inline uint16_t parity (uint16_t x)
> +{
> +    uint16_t i;
> +    for (i = 4 * sizeof (x); i > 0; i /= 2)
> +        x ^= x >> i;
> +    return x & 1;
> +}
> +
> +static inline void clear_parity_bits(uint16_t *buf, int len) {
> +    int i;
> +    for (i = 0; i < len; i++)
> +        buf[i] &= 0xff;
> +}
> +
> +static int check_vanc_parity_checksum(uint16_t *buf, int len, uint16_t checksum) {
> +    int i;
> +    uint32_t vanc_sum;

uint16_t is enough, it does not matter if it overflows. Also don't forget to
initialize this to 0, the previous patch had that.

> +    for (i = 3; i < len - 1; i++) {
> +        uint16_t v = buf[i];
> +        int np = v >> 8;
> +        int p = parity(v & 0xff);
> +        if ((!!p ^ !!(v & 0x100)) || (np != 1 && np != 2)) {
> +            // Parity check failed
> +            return -1;
> +        }
> +        vanc_sum += v;
> +    }
> +    vanc_sum &= 0x1ff;
> +    vanc_sum |= ((~vanc_sum & 0x100) << 1);
> +    if (checksum != vanc_sum) {
> +        // Checksum verification failed
> +        return -1;
> +    }
> +    return 0;
> +}
> +
>  /* The 10-bit VANC data is packed in V210, we only need the luma component. */
>  static void extract_luma_from_v210(uint16_t *dst, const uint8_t *src, int width)
>  {
> @@ -232,19 +269,148 @@ static uint8_t* teletext_data_unit_from_ancillary_packet(uint16_t *py, uint16_t
>      return tgt;
>  }
> 
> -static uint8_t* teletext_data_unit_from_vanc_data(uint16_t *py, uint8_t *tgt, int64_t wanted_lines)
> +uint8_t *vanc_to_cc(AVFormatContext *avctx, uint16_t *buf, size_t words,
> +    unsigned &cc_count)
>  {
> -    uint16_t *pend = py + 1920;
> +    size_t i, len = (buf[5] & 0xff) + 6 + 1;
> +    uint8_t cdp_sum, rate;
> +    uint16_t hdr, ftr;
> +    uint8_t *cc;
> +    uint16_t *cdp = &buf[6]; // CDP follows
> +    if (cdp[0] != 0x96 || cdp[1] != 0x69) {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid CDP header 0x%.2x 0x%.2x", cdp[0], cdp[1]);
> +        return NULL;
> +    }
> 
> -    while (py < pend - 6) {
> -        if (py[0] == 0 && py[1] == 0x3ff && py[2] == 0x3ff) {           // ancillary data flag
> -            py += 3;
> -            tgt = teletext_data_unit_from_ancillary_packet(py, pend, tgt, wanted_lines, 0);
> -            py += py[2] & 255;
> +    len -= 7; // remove VANC header and checksum
> +
> +    if (cdp[2] != len) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP len %d != %zu", cdp[2], len);
> +        return NULL;
> +    }
> +
> +    cdp_sum = 0;
> +    for (i = 0; i < len - 1; i++)
> +        cdp_sum += cdp[i];
> +    cdp_sum = cdp_sum ? 256 - cdp_sum : 0;
> +    if (cdp[len - 1] != cdp_sum) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP checksum invalid 0x%.4x != 0x%.4x", cdp_sum, cdp[len-1]);
> +        return NULL;
> +    }
> +
> +    rate = cdp[3];
> +    if (!(rate & 0x0f)) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP frame rate invalid (0x%.2x)", rate);
> +        return NULL;
> +    }
> +    rate >>= 4;
> +    if (rate > 8) {
> +        av_log(avctx, AV_LOG_WARNING, "CDP frame rate invalid (0x%.2x)", rate);
> +        return NULL;
> +    }
> +
> +    if (!(cdp[4] & 0x43)) /* ccdata_present | caption_service_active | reserved */ {
> +        av_log(avctx, AV_LOG_WARNING, "CDP flags invalid (0x%.2x)", cdp[4]);
> +        return NULL;
> +    }
> +
> +    hdr = (cdp[5] << 8) | cdp[6];
> +    if (cdp[7] != 0x72) /* ccdata_id */ {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid ccdata_id 0x%.2x", cdp[7]);
> +        return NULL;
> +    }
> +
> +    cc_count = cdp[8];
> +    if (!(cc_count & 0xe0)) {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid cc_count 0x%.2x", cc_count);
> +        return NULL;
> +    }
> +
> +    cc_count &= 0x1f;
> +    if ((len - 13) < cc_count * 3) {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid cc_count %d (> %zu)", cc_count * 3, len - 13);
> +        return NULL;
> +    }
> +
> +    if (cdp[len - 4] != 0x74) /* footer id */ {
> +        av_log(avctx, AV_LOG_WARNING, "Invalid footer id 0x%.2x", cdp[len-4]);
> +        return NULL;
> +    }
> +
> +    ftr = (cdp[len - 3] << 8) | cdp[len - 2];
> +    if (ftr != hdr) {
> +        av_log(avctx, AV_LOG_WARNING, "Header 0x%.4x != Footer 0x%.4x", hdr, ftr);
> +        return NULL;
> +    }
> +
> +    cc = (uint8_t *)av_malloc(cc_count * 3);
> +    if (cc == NULL) {
> +        av_log(avctx, AV_LOG_WARNING, "CC - av_malloc failed for cc_count = %d", cc_count);

You should add "\n" to the end of all these warnings.

> +        return NULL;
> +    }
> +
> +    for (size_t i = 0; i < cc_count; i++) {
> +        cc[3*i + 0] = cdp[9 + 3*i+0] /* & 3 */;
> +        cc[3*i + 1] = cdp[9 + 3*i+1];
> +        cc[3*i + 2] = cdp[9 + 3*i+2];
> +    }
> +
> +    cc_count *= 3;
> +    return cc;
> +}
> +
> +uint8_t *get_metadata(AVFormatContext *avctx, uint16_t *buf, size_t width,
> +                      uint8_t *tgt, size_t tgt_size, AVPacket *pkt)
> +{
> +    decklink_cctx *cctx = (struct decklink_cctx *) avctx->priv_data;
> +    uint16_t *max_buf = buf + width;
> +
> +    while (buf < max_buf - 6) {
> +        int len;
> +        uint16_t did = buf[3] & 0xFF;                                  // data id
> +        uint16_t sdid = buf[4] & 0xFF;                                 // secondary data id
> +        /* Check for VANC header */
> +        if (buf[0] != 0 || buf[1] != 0x3ff || buf[2] != 0x3ff) {
> +            return tgt;
> +        }
> +
> +        len = (buf[5] & 0xff) + 6 + 1;
> +        if (len > max_buf - buf) {
> +            av_log(avctx, AV_LOG_WARNING, "Data Count (%d) > data left (%zu)\n",
> +                    len, max_buf - buf);
> +            return tgt;
> +        }
> +
> +        if (did == 0x43 && (sdid == 0x02 || sdid == 0x03) && cctx->teletext_lines &&
> +            width == 1920 && tgt_size >= 1920) {
> +            if (!check_vanc_parity_checksum(buf, len, buf[len - 1])) {

The check is inverted. Use the common ffmpeg practice: if (function() < 0) that means error.

> +                av_log(avctx, AV_LOG_WARNING, "VANC parity or checksum incorrect \n");

no need for extra space before \n.

> +                goto skip_packet;
> +            }
> +            tgt = teletext_data_unit_from_ancillary_packet(buf + 3, buf + len, tgt, cctx->teletext_lines, 0);
> +        } else if (did == 0x61 && sdid == 0x01) {
> +            unsigned int data_len;
> +            uint8_t *data;
> +            if (!check_vanc_parity_checksum(buf, len, buf[len - 1])) {

The check is inverted. Use the common ffmpeg practice: if (function() < 0) that means error.

Also please test your patches with real CC data before posting them, I 
know it is tedious work with all the patch iterations, but I do the same 
with teletext.

> +                av_log(avctx, AV_LOG_WARNING, "VANC parity or checksum incorrect \n");

no need for extra space before \n.

> +                goto skip_packet;
> +            }
> +            clear_parity_bits(buf, len);
> +            data = vanc_to_cc(avctx, buf, width, data_len);
> +            if (data) {
> +                uint8_t *pkt_cc = av_packet_new_side_data(pkt, AV_PKT_DATA_A53_CC, data_len);
> +                if (pkt_cc)
> +                    memcpy(pkt_cc, data, data_len);
> +                av_free(data);
> +            }
>          } else {
> -            py++;
> +            av_log(avctx, AV_LOG_DEBUG, "Unknown meta data DID = 0x%.2x SDID = 0x%.2x\n",
> +                    did, sdid);
>          }
> +skip_packet:
> +        buf += len;
>      }
> +
>      return tgt;
>  }
> 
> @@ -536,7 +702,7 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                             videoFrame->GetHeight();
>          //fprintf(stderr,"Video Frame size %d ts %d\n", pkt.size, pkt.pts);
> 
> -        if (!no_video && ctx->teletext_lines) {
> +        if (!no_video) {
>              IDeckLinkVideoFrameAncillary *vanc;
>              AVPacket txt_pkt;
>              uint8_t txt_buf0[3531]; // 35 * 46 bytes decoded teletext lines + 1 byte data_identifier + 1920 bytes OP47 decode buffer
> @@ -549,7 +715,8 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                  txt_buf[0] = 0x10;    // data_identifier - EBU_data
>                  txt_buf++;
>  #if CONFIG_LIBZVBI
> -                if (ctx->bmd_mode == bmdModePAL && (vanc_format == bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) {
> +                if (ctx->bmd_mode == bmdModePAL && ctx->teletext_lines &&
> +                    (vanc_format == bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) {
>                      av_assert0(videoFrame->GetWidth() == 720);
>                      for (i = 6; i < 336; i++, line_mask <<= 1) {
>                          uint8_t *buf;
> @@ -571,13 +738,8 @@ HRESULT decklink_input_callback::VideoInputFrameArrived(
>                          if (vanc->GetBufferForVerticalBlankingLine(i, (void**)&buf) == S_OK) {
>                              uint16_t luma_vanc[MAX_WIDTH_VANC];
>                              extract_luma_from_v210(luma_vanc, buf, videoFrame->GetWidth());
> -                            if (videoFrame->GetWidth() == 1920) {
> -                                txt_buf = teletext_data_unit_from_vanc_data(luma_vanc, 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;
> -                                }
> -                            }
> +                            txt_buf = get_metadata(avctx, luma_vanc, videoFrame->GetWidth(),
> +                                                   txt_buf, sizeof(txt_buf0) - (txt_buf - txt_buf0), &pkt);
>                          }
>                          if (i == vanc_line_numbers[idx].field0_vanc_end)
>                              i = vanc_line_numbers[idx].field1_vanc_start - 1;
> -- 
> 1.9.1
>

Fingers crossed for the next patch version :)

Thanks,
Marton
Jeyapal, Karthick Sept. 28, 2017, 4:17 a.m. UTC | #12 
>Sorry, still not quite there... Here are some further comments:

Thanks for your comments and patience. Please find the updated patch attached, with all the comments taken.
Also, I have rebased this patch with the latest ffmpeg, as avcodec version numbers have changed in the meantime.

>Also please test your patches with real CC data before posting them, I

>know it is tedious work with all the patch iterations, but I do the same

>with teletext.

Sorry about that. I did test that patch with real CC data and corrected the inverted checks, but I forgot and attached the older patch in a hurry.
Sorry for wasting your time there.

>Fingers crossed for the next patch version :)

Well, me too :)

Thanks and Regards,
Karthick
Marton Balint Sept. 28, 2017, 8:16 p.m. UTC | #13 
On Thu, 28 Sep 2017, Jeyapal, Karthick wrote:

>> Sorry, still not quite there... Here are some further comments:
> Thanks for your comments and patience. Please find the updated patch attached, with all the comments taken.
> Also, I have rebased this patch with the latest ffmpeg, as avcodec version numbers have changed in the meantime.
>
>> Also please test your patches with real CC data before posting them, I
>> know it is tedious work with all the patch iterations, but I do the same
>> with teletext.
> Sorry about that. I did test that patch with real CC data and corrected the inverted checks, but I forgot and attached the older patch in a hurry.
> Sorry for wasting your time there.
>
>> Fingers crossed for the next patch version :)
> Well, me too :)

Applied the series, thanks!

Marton
James Almer Sept. 29, 2017, 2 a.m. UTC | #14 
On 9/28/2017 1:17 AM, Jeyapal, Karthick wrote:
>> Sorry, still not quite there... Here are some further comments:
> Thanks for your comments and patience. Please find the updated patch attached, with all the comments taken.
> Also, I have rebased this patch with the latest ffmpeg, as avcodec version numbers have changed in the meantime.
> 
>> Also please test your patches with real CC data before posting them, I
>> know it is tedious work with all the patch iterations, but I do the same
>> with teletext.
> Sorry about that. I did test that patch with real CC data and corrected the inverted checks, but I forgot and attached the older patch in a hurry.
> Sorry for wasting your time there.
> 
>> Fingers crossed for the next patch version :)
> Well, me too :)
> 
> Thanks and Regards,
> Karthick

> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 39d9ad9..7aee6f9 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -29,7 +29,7 @@
>  
>  #define LIBAVCODEC_VERSION_MAJOR  57
>  #define LIBAVCODEC_VERSION_MINOR 106
> -#define LIBAVCODEC_VERSION_MICRO 103
> +#define LIBAVCODEC_VERSION_MICRO 104
>  
>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>                                                 LIBAVCODEC_VERSION_MINOR, \
> diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
> index 38e6bd8..3c283e0 100644
> --- a/libavdevice/decklink_dec.cpp
> +++ b/libavdevice/decklink_dec.cpp
> @@ -1,6 +1,7 @@
>  /*
>   * Blackmagic DeckLink input
>   * Copyright (c) 2013-2014 Luca Barbato, Deti Fliegl
> + * Copyright (c) 2014 Rafaël Carré
>   * Copyright (c) 2017 Akamai Technologies, Inc.
>   *
>   * This file is part of FFmpeg.
> @@ -105,6 +106,42 @@ static int get_vanc_line_idx(BMDDisplayMode mode)
>      return i - 1;
>  }
>  
> +static inline uint16_t parity (uint16_t x)
> +{
> +    uint16_t i;
> +    for (i = 4 * sizeof (x); i > 0; i /= 2)
> +        x ^= x >> i;
> +    return x & 1;
> +}

Can't you use av_parity() instead?
Jeyapal, Karthick Sept. 29, 2017, 3:12 a.m. UTC | #15 
>Applied the series, thanks!

>

>Marton


Thanks a lot!

Regards,
Karthick
Jeyapal, Karthick Sept. 29, 2017, 3:38 a.m. UTC | #16 
>On 9/29/17, 7:36 AM, "James Almer" <jamrial@gmail.com> wrote:

>>  

>> +static inline uint16_t parity (uint16_t x)

>> +{

>> +    uint16_t i;

>> +    for (i = 4 * sizeof (x); i > 0; i /= 2)

>> +        x ^= x >> i;

>> +    return x & 1;

>> +}

>

>Can't you use av_parity() instead?


Yes, I can. Thanks for pointing it out. But that patch has been submitted already. Hence, I have attached a fresh patch with this suggested change.

Regards,
Karthick
Carl Eugen Hoyos Oct. 1, 2017, 4:28 p.m. UTC | #17 
2017-09-29 5:38 GMT+02:00 Jeyapal, Karthick <kjeyapal@akamai.com>:
>>On 9/29/17, 7:36 AM, "James Almer" <jamrial@gmail.com> wrote:
>>>
>>> +static inline uint16_t parity (uint16_t x)
>>> +{
>>> +    uint16_t i;
>>> +    for (i = 4 * sizeof (x); i > 0; i /= 2)
>>> +        x ^= x >> i;
>>> +    return x & 1;
>>> +}
>>
>>Can't you use av_parity() instead?
>
> Yes, I can. Thanks for pointing it out. But that patch has been submitted
> already. Hence, I have attached a fresh patch with this suggested change.

Pushed assuming you have tested this.

Thank you, Carl Eugen
diff mbox

Patch

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 513236a..d4a458f 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -1600,6 +1600,13 @@  enum AVPacketSideDataType {
     AV_PKT_DATA_CONTENT_LIGHT_LEVEL,
 
     /**
+     * ATSC A53 Part 4 Closed Captions. This metadata should be associated with
+     * a video stream. A53 CC bitstream is stored as uint8_t in AVPacketSideData.data.
+     * The number of bytes of CC data is AVPacketSideData.size.
+     */
+    AV_PKT_DATA_A53_CC,
+
+    /**
      * The number of side data elements (in fact a bit more than it).
      * This is not part of the public API/ABI in the sense that it may
      * change when new side data types are added.
diff --git a/libavdevice/decklink_dec.cpp b/libavdevice/decklink_dec.cpp
index 0b88fc8..23e1b35 100644
--- a/libavdevice/decklink_dec.cpp
+++ b/libavdevice/decklink_dec.cpp
@@ -106,6 +106,13 @@  static void get_vanc_lines(int bmd_field_dominance, int height, int *field0_vanc
     }
 }
 
+static inline unsigned parity (unsigned x)
+{
+    for (unsigned i = 4 * sizeof (x); i > 0; i /= 2)
+        x ^= x >> i;
+    return x & 1;
+}
+
 /* The 10-bit VANC data is packed in V210, we only need the luma component. */
 static void extract_luma_from_v210(uint16_t *dst, const uint8_t *src, int width)
 {
@@ -249,6 +256,152 @@  static uint8_t* teletext_data_unit_from_vanc_data(uint16_t *py, uint8_t *tgt, in
     return tgt;
 }
 
+uint8_t *vanc_to_cc(AVFormatContext *avctx, uint16_t *buf, size_t words,
+    unsigned &cc_count)
+{
+    size_t len = (buf[5] & 0xff) + 6 + 1;
+
+    /* CDP follows */
+    uint16_t *cdp = &buf[6];
+    if (cdp[0] != 0x96 || cdp[1] != 0x69) {
+        av_log(avctx, AV_LOG_WARNING, "Invalid CDP header 0x%.2x 0x%.2x", cdp[0], cdp[1]);
+        return NULL;
+    }
+
+    len -= 7; // remove VANC header and checksum
+
+    if (cdp[2] != len) {
+        av_log(avctx, AV_LOG_WARNING, "CDP len %d != %zu", cdp[2], len);
+        return NULL;
+    }
+
+    uint8_t cdp_sum = 0;
+    for (size_t i = 0; i < len - 1; i++)
+        cdp_sum += cdp[i];
+    cdp_sum = cdp_sum ? 256 - cdp_sum : 0;
+    if (cdp[len - 1] != cdp_sum) {
+        av_log(avctx, AV_LOG_WARNING, "CDP checksum invalid 0x%.4x != 0x%.4x", cdp_sum, cdp[len-1]);
+        return NULL;
+    }
+
+    uint8_t rate = cdp[3];
+    if (!(rate & 0x0f)) {
+        av_log(avctx, AV_LOG_WARNING, "CDP frame rate invalid (0x%.2x)", rate);
+        return NULL;
+    }
+    rate >>= 4;
+    if (rate > 8) {
+        av_log(avctx, AV_LOG_WARNING, "CDP frame rate invalid (0x%.2x)", rate);
+        return NULL;
+    }
+
+    if (!(cdp[4] & 0x43)) /* ccdata_present | caption_service_active | reserved */ {
+        av_log(avctx, AV_LOG_WARNING, "CDP flags invalid (0x%.2x)", cdp[4]);
+        return NULL;
+    }
+
+    uint16_t hdr = (cdp[5] << 8) | cdp[6];
+    if (cdp[7] != 0x72) /* ccdata_id */ {
+        av_log(avctx, AV_LOG_WARNING, "Invalid ccdata_id 0x%.2x", cdp[7]);
+        return NULL;
+    }
+
+    cc_count = cdp[8];
+    if (!(cc_count & 0xe0)) {
+        av_log(avctx, AV_LOG_WARNING, "Invalid cc_count 0x%.2x", cc_count);
+        return NULL;
+    }
+
+    cc_count &= 0x1f;
+    if ((len - 13) < cc_count * 3) {
+        av_log(avctx, AV_LOG_WARNING, "Invalid cc_count %d (> %zu)", cc_count * 3, len - 13);
+        return NULL;
+    }
+
+    if (cdp[len - 4] != 0x74) /* footer id */ {
+        av_log(avctx, AV_LOG_WARNING, "Invalid footer id 0x%.2x", cdp[len-4]);
+        return NULL;
+    }
+
+    uint16_t ftr = (cdp[len - 3] << 8) | cdp[len - 2];
+    if (ftr != hdr) {
+        av_log(avctx, AV_LOG_WARNING, "Header 0x%.4x != Footer 0x%.4x", hdr, ftr);
+        return NULL;
+    }
+
+    uint8_t *cc = (uint8_t *)av_malloc(cc_count * 3);
+
+    for (size_t i = 0; i < cc_count; i++) {
+        cc[3*i + 0] = cdp[9 + 3*i+0] /* & 3 */;
+        cc[3*i + 1] = cdp[9 + 3*i+1];
+        cc[3*i + 2] = cdp[9 + 3*i+2];
+    }
+
+    cc_count *= 3;
+    return cc;
+}
+
+uint8_t *get_metadata(AVFormatContext *avctx, uint16_t *buf, size_t width,
+                      uint8_t *tgt, size_t tgt_size, AVPacket *pkt)
+{
+    decklink_cctx *cctx = (struct decklink_cctx *) avctx->priv_data;
+    uint16_t did = buf[3] & 0xFF;                                  // data id
+    uint16_t sdid = buf[4] & 0xFF;                                 // secondary data id
+
+    static const uint8_t vanc_header[6] = { 0x00, 0x00, 0xff, 0x03, 0xff, 0x03 };
+    if (memcmp(vanc_header, buf, 3 * 2)) {
+        /* Does not start with the VANC header */
+        return tgt;
+    }
+
+    size_t len = (buf[5] & 0xff) + 6 + 1;
+    if (len > width) {
+        av_log(avctx, AV_LOG_WARNING, "Data Count (%zu) > line length (%zu)\n",
+                len, width);
+        return tgt;
+    }
+
+    uint16_t vanc_sum = 0;
+    for (size_t i = 3; i < len - 1; i++) {
+        uint16_t v = buf[i];
+        int np = v >> 8;
+        int p = parity(v & 0xff);
+        if ((!!p ^ !!(v & 0x100)) || (np != 1 && np != 2)) {
+            av_log(avctx, AV_LOG_WARNING, "Parity incorrect for word %zu\n", i);
+            return tgt;
+        }
+        vanc_sum += v;
+        vanc_sum &= 0x1ff;
+        buf[i] &= 0xff;
+    }
+
+    vanc_sum |= ((~vanc_sum & 0x100) << 1);
+    if (buf[len - 1] != vanc_sum) {
+        av_log(avctx, AV_LOG_WARNING,
+                "VANC checksum incorrect: 0x%.4x != 0x%.4x\n", vanc_sum,
+                buf[len - 1]);
+        return tgt;
+    }
+
+    if (did == 0x43 && (sdid == 0x02 || sdid == 0x03) && cctx->teletext_lines &&
+        width == 1920 && tgt_size >= 1920) {
+        tgt = teletext_data_unit_from_vanc_data(buf, tgt, cctx->teletext_lines);
+    } else if (did == 0x61 && sdid == 0x01) {
+        unsigned int data_len;
+        uint8_t *data = vanc_to_cc(avctx, buf, width, data_len);
+        if (data) {
+            uint8_t *pkt_cc = av_packet_new_side_data(pkt, AV_PKT_DATA_A53_CC, data_len);
+            memcpy(pkt_cc, data, data_len);
+            av_free(data);
+        }
+    } else {
+        av_log(avctx, AV_LOG_WARNING, "Unknown meta data DID = 0x%.2x SDID = 0x%.2x\n",
+                did, sdid);
+    }
+
+    return tgt;
+}
+
 static void avpacket_queue_init(AVFormatContext *avctx, AVPacketQueue *q)
 {
     struct decklink_cctx *ctx = (struct decklink_cctx *)avctx->priv_data;
@@ -537,7 +690,7 @@  HRESULT decklink_input_callback::VideoInputFrameArrived(
                            videoFrame->GetHeight();
         //fprintf(stderr,"Video Frame size %d ts %d\n", pkt.size, pkt.pts);
 
-        if (!no_video && ctx->teletext_lines) {
+        if (!no_video) {
             IDeckLinkVideoFrameAncillary *vanc;
             AVPacket txt_pkt;
             uint8_t txt_buf0[3531]; // 35 * 46 bytes decoded teletext lines + 1 byte data_identifier + 1920 bytes OP47 decode buffer
@@ -550,7 +703,8 @@  HRESULT decklink_input_callback::VideoInputFrameArrived(
                 txt_buf[0] = 0x10;    // data_identifier - EBU_data
                 txt_buf++;
 #if CONFIG_LIBZVBI
-                if (ctx->bmd_mode == bmdModePAL && (vanc_format == bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) {
+                if (ctx->bmd_mode == bmdModePAL && ctx->teletext_lines &&
+                    (vanc_format == bmdFormat8BitYUV || vanc_format == bmdFormat10BitYUV)) {
                     av_assert0(videoFrame->GetWidth() == 720);
                     for (i = 6; i < 336; i++, line_mask <<= 1) {
                         uint8_t *buf;
@@ -575,13 +729,8 @@  HRESULT decklink_input_callback::VideoInputFrameArrived(
                         if (vanc->GetBufferForVerticalBlankingLine(i, (void**)&buf) == S_OK) {
                             uint16_t luma_vanc[4096]; // Allocated for highest width (4K)
                             extract_luma_from_v210(luma_vanc, buf, videoFrame->GetWidth());
-                            if (videoFrame->GetWidth() == 1920) {
-                                txt_buf = teletext_data_unit_from_vanc_data(luma_vanc, 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;
-                                }
-                            }
+                            txt_buf = get_metadata(avctx, luma_vanc, videoFrame->GetWidth(),
+                                                   txt_buf, 3531 - (txt_buf - txt_buf0), &pkt);
                         }
                         if (i == field0_vanc_end)
                             i = field1_vanc_start;