[FFmpeg-devel] [PATCH 2/2] Adding closed caption decoder

[Michael Niedermayer]

On Wed, Dec 31, 2014 at 07:09:33PM +0530, Anshul wrote:
[...]

> +static void build_parity_table(int *parity_table)
> +{
> +    unsigned int byte;
> +    int parity_v;
> +    for (byte = 0; byte <= 127; byte++) {
> +        parity_v = av_popcount(byte) & 1;
> +        parity_table[byte] = parity_v;
> +        parity_table[byte | 0x80] = !parity_v;
> +    }
> +}

This should not be needed, av_popcount(byte) & 1 could be used
directly or you could use something like this: (untested)
(0x6996 >> ((byte ^ (byte>>4)) & 15)) & 1

the code using the parity stuff does not seem speed critical
but maybe iam missing something ?

[...]

> +static void handle_pac( CCaptionSubContext *ctx, uint8_t hi, uint8_t lo )
> +{
> +    static const uint8_t row_map[] = {
> +        11, -1, 1, 2, 3, 4, 12, 13, 14, 15, 5, 6, 7, 8, 9, 10
> +    };

you are having negative values in a unsigned 8bit table
i assume either of these is not intended


[...]
> +static int process_cc608(CCaptionSubContext *ctx, int64_t pts, uint8_t hi, uint8_t lo)
> +{
> +    int ret = 0;
> +#define COR3(var, with1, with2, with3)  ( (var) == (with1) ||  (var) == (with2) || (var) == (with3) )
> +    if ( hi == ctx->prev_cmd[0] && lo == ctx->prev_cmd[1]) {
> +    /* ignore redundant command */
> +    } else if ( (hi == 0x10 && (lo >= 0x40 || lo <= 0x5f)) ||
> +              ( (hi >= 0x11 && hi <= 0x17) && (lo >= 0x40 && lo <= 0x7f) ) ) {
> +        handle_pac(ctx, hi, lo);
> +    } else if ( COR3(hi, 0x14, 0x15, 0x1C) && lo == 0x20 ) {
> +    /* resume caption loading */
> +        ctx->mode = CCMODE_POPON;
> +    } else if ( COR3(hi, 0x14, 0x15, 0x1C) && lo == 0x25 ) {
> +        ctx->rollup = 2;
> +    } else if ( COR3(hi, 0x14, 0x15, 0x1C) && lo == 0x26 ) {
> +        ctx->rollup = 3;
> +    } else if ( COR3(hi, 0x14, 0x15, 0x1C) && lo == 0x27 ) {
> +        ctx->rollup = 4;
> +    } else if ( COR3(hi, 0x14, 0x15, 0x1C) && lo == 0x29 ) {
> +    /* resume direct captioning */
> +        ctx->mode = CCMODE_PAINTON;
> +    } else if ( COR3(hi, 0x14, 0x15, 0x1C) && lo == 0x2C ) {
> +    /* erase display memory */
> +        ret = handle_edm(ctx, pts);
> +    } else if ( COR3(hi, 0x14, 0x15, 0x1C) && lo == 0x2D ) {
> +    /* carriage return */
> +        av_log(ctx, AV_LOG_DEBUG,"cdp (handle cr)\n");
> +        ctx->row_cnt++;
> +        if(ctx->row_cnt == ctx->rollup) {
> +            ctx->row_cnt = 0;
> +            ret = handle_edm(ctx, pts);
> +            ctx->active_screen = !ctx->active_screen;
> +        }
> +    } else if ( COR3(hi, 0x14, 0x15, 0x1C) && lo == 0x2F ) {
> +    /* end of caption */
> +        ret = handle_eoc(ctx, pts);
> +    } else if (hi>=0x20) {
> +    /* Standard characters (always in pairs) */
> +        handle_char(ctx, hi, lo, pts);
> +    } else {
> +    /* Ignoring all other non data code */
> +    }
> +
> +    /* set prev command */
> +     ctx->prev_cmd[0] = hi;
> +     ctx->prev_cmd[1] = lo;
> +
> +#undef COR3
> +    return ret;
> +
> +}
> +static int decode(AVCodecContext *avctx, void *data, int *got_sub, AVPacket *avpkt)
> +{
> +    CCaptionSubContext *ctx = avctx->priv_data;
> +    AVSubtitle *sub = data;

> +    uint8_t *bptr = avpkt->data;

The input packets data is read only unless you do something
(dup/memcpy/whatever) so this should be const


> +    int len = avpkt->size;
> +    int ret = 0;
> +    int i;
> +
> +    for (i  = 0; i < len; i += 3) {
> +        uint8_t cc_type = *(bptr + i) & 3;
> +        if (validate_cc_data_pair( bptr + i, ctx->parity_table ) )
> +            continue;
> +        /* ignoring data field 1 */
> +        if(cc_type == 1)
> +            continue;
> +        else
> +            process_cc608(ctx, avpkt->pts, *(bptr + i + 1), *(bptr + i + 2));
> +    }
> +    if(ctx->erase_display_memory && *ctx->buffer.str)
> +    {
> +        int start_time = av_rescale_q(ctx->start_time, avctx->time_base, (AVRational){ 1, 100 });
> +        int end_time = av_rescale_q(ctx->end_time, avctx->time_base, (AVRational){ 1, 100 });

> +	av_log(ctx, AV_LOG_DEBUG,"cdp writing data (%s)\n",ctx->buffer.str);

tabs, also theres a alot of deug av_logs in the code, it might make
sense to remove or disable these before its pushed


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 2
"100% positive feedback" - "All either got their money back or didnt complain"
"Best seller ever, very honest" - "Seller refunded buyer after failed scam"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150102/bbfc5f69/attachment.asc>