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

[Anshul]

On 01/03/2015 01:42 AM, Michael Niedermayer wrote:
> 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 ?
parity is checked for each byte of data in closed caption, so I thought
it would be speed critical. only 7 bits are used in each byte. 1 bit is
fr parity.
> [...]
>
>> +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
done, -1 was needed.
changed uint8_t to int8_t
>
> [...]
>> +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
copied data in AVbufferRef.
>
>> +    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
most of the logs I have removed, others I have disabled.

I have attached 2 pacth 1 improvement of this other is info regarding
1st one.
-Anshul
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Adding-Closed-caption-Support.patch
Type: text/x-patch
Size: 13063 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150103/9d3b3d3b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Adding-Closed-caption-accessories.patch
Type: text/x-patch
Size: 1634 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150103/9d3b3d3b/attachment-0001.bin>