On 12/06/2014 04:24 PM, Anshul wrote: > On 12/06/2014 04:22 PM, Anshul wrote: >> On 12/05/2014 07:56 PM, Nicolas George wrote: >>> Hi. I had time to look at the code with some more details. Comments are >>> below. >>> >>>> >From 31f69ccfb45247a7cc203084a931b8523284aa13 Mon Sep 17 00:00:00 2001 >>>> From: Anshul Maheshwari <anshul.ffm...@gmail.com> >>>> Date: Wed, 3 Dec 2014 23:37:22 +0530 >>>> Subject: [PATCH 2/2] Adding Closed caption Decoder >>>> >>>> --- >>>> libavcodec/Makefile | 1 + >>>> libavcodec/allcodecs.c | 1 + >>>> libavcodec/ccaption_dec.c | 318 >>>> ++++++++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 320 insertions(+) >>>> create mode 100644 libavcodec/ccaption_dec.c >>>> >>>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile >>>> index fa0f53d..bbc516d 100644 >>>> --- a/libavcodec/Makefile >>>> +++ b/libavcodec/Makefile >>>> @@ -173,6 +173,7 @@ OBJS-$(CONFIG_BRENDER_PIX_DECODER) += brenderpix.o >>>> OBJS-$(CONFIG_C93_DECODER) += c93.o >>>> OBJS-$(CONFIG_CAVS_DECODER) += cavs.o cavsdec.o cavsdsp.o \ >>>> cavsdata.o mpeg12data.o >>>> +OBJS-$(CONFIG_CCAPTION_DECODER) += ccaption_dec.o >>>> OBJS-$(CONFIG_CDGRAPHICS_DECODER) += cdgraphics.o >>>> OBJS-$(CONFIG_CDXL_DECODER) += cdxl.o >>>> OBJS-$(CONFIG_CINEPAK_DECODER) += cinepak.o >>>> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c >>>> index 0d39d33..8c07388 100644 >>>> --- a/libavcodec/allcodecs.c >>>> +++ b/libavcodec/allcodecs.c >>>> @@ -480,6 +480,7 @@ void avcodec_register_all(void) >>>> /* subtitles */ >>>> REGISTER_ENCDEC (SSA, ssa); >>>> REGISTER_ENCDEC (ASS, ass); >>>> + REGISTER_DECODER(CCAPTION, ccaption); >>>> REGISTER_ENCDEC (DVBSUB, dvbsub); >>>> REGISTER_ENCDEC (DVDSUB, dvdsub); >>>> REGISTER_DECODER(JACOSUB, jacosub); >>>> diff --git a/libavcodec/ccaption_dec.c b/libavcodec/ccaption_dec.c >>>> new file mode 100644 >>>> index 0000000..0a7dfd8 >>>> --- /dev/null >>>> +++ b/libavcodec/ccaption_dec.c >>>> @@ -0,0 +1,318 @@ >>>> +/* >>>> + * Closed Caption Decoding >>>> + * Copyright (c) 2014 Anshul Maheshwari >>>> + * >>>> + * This file is part of FFmpeg. >>>> + * >>>> + * FFmpeg is free software; you can redistribute it and/or >>>> + * modify it under the terms of the GNU Lesser General Public >>>> + * License as published by the Free Software Foundation; either >>>> + * version 2.1 of the License, or (at your option) any later version. >>>> + * >>>> + * FFmpeg is distributed in the hope that it will be useful, >>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>>> + * Lesser General Public License for more details. >>>> + * >>>> + * You should have received a copy of the GNU Lesser General Public >>>> + * License along with FFmpeg; if not, write to the Free Software >>>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >>>> 02110-1301 USA >>>> + */ >>>> + >>>> +#include "avcodec.h" >>>> +#include "ass.h" >>>> + >>>> +#define SCREEN_ROWS 15 >>>> +#define SCREEN_COLUMNS 32 >>>> + >>>> +#define SET_FLAG(var, val) ( var |= ( 1 << (val) ) ) >>>> +#define UNSET_FLAG(var, val) ( var &= ~( 1 << (val)) ) >>>> +#define CHECK_FLAG(var, val) ( (var) & (1 << (val) ) ) >>>> + >>>> +enum cc_mode { >>>> + CCMODE_POPON, >>>> + CCMODE_PAINTON, >>>> + CCMODE_ROLLUP_2, >>>> + CCMODE_ROLLUP_3, >>>> + CCMODE_ROLLUP_4, >>>> + CCMODE_TEXT, >>>> +}; >>>> + >>>> +struct Screen { >>>> + uint8_t characters[SCREEN_ROWS][SCREEN_COLUMNS+1]; >>> Maybe add a comment about the +1? >> done >>>> + /* >>>> + * row used flag will be 0 when none in use other wise it will have >>>> its >>>> + * corrosponding bit high. >>> Language nit. I suggest: "Bitmask of used rows; if a bit is not set, the >>> corresponding row is not used." >> done >>>> + * for setting row 1 use row | (1 >> 1) >>>> + * for setting row 15 use row | (1 >> 15) >>> Are you sure that is ">>" and not "<<"? And is it a good idea to number >>> starting from 1? >> It was just commented in wrong way, corrected comment. >>>> + */ >>>> + int16_t row_used; >>>> +}; >>>> + >>>> + >>>> +typedef struct CCaptionSubContext { >>>> + int parity_table[256]; >>>> + int row_cnt; >>>> + struct Screen screen[2]; >>>> + int active_screen; >>>> + int cursor_row; >>>> + int cursor_column; >>>> + AVBPrint buffer; >>>> + /* erase display memory */ >>>> + int edm; >>> It is used only a handful of times: I suggest a more meaningful name instead >>> of a comment: "erase_disp_mem" for example. >> done >>>> + int rollup; >>>> + enum cc_mode mode; >>>> + int64_t start_time; >>>> + /* visible screen time */ >>>> + int64_t startv_time; >>> Is the v a typo? >> no it was written to express v = visible, (visible screen start time) >>>> + int64_t end_time; >>>> + char prev_cmd[2]; >>> The code uses various types for these values: char, unsigned char, uint8_t. >>> I suggest to stick with uint8_t if it works. >> changed unsigned char to uint8_t >>>> +}CCaptionSubContext; >>>> + >>>> +static void build_parity_table(int *parity_table) >>>> +{ >>>> + unsigned int byte; >>> Inconsistent indentation. >> corrected >>>> + int parity_v; >>>> + for (byte = 0; byte <= 127; byte++) { >>>> + parity_v = av_popcount(byte & 0x7f) & 1; >>> The & 0x7f is redundant. >> removed >>>> + parity_table[byte] = parity_v; >>>> + parity_table[byte | 0x80] = !parity_v; >>>> + } >>>> +} >>>> + >>>> +static av_cold int init_decoder(AVCodecContext *avctx) >>>> +{ >>>> + >>>> + CCaptionSubContext *ctx = avctx->priv_data; >>>> + >>>> + build_parity_table(ctx->parity_table); >>>> + ctx->row_cnt = 0; >>>> + av_bprint_init(&ctx->buffer, 0, AV_BPRINT_SIZE_UNLIMITED); >>>> + ctx->edm = 0; >>>> + /* taking by default roll up to 2 */ >>>> + ctx->rollup = 2; >>>> + /* set active screen as 0 */ >>>> + ctx->active_screen = 0; >>>> + memset(ctx->prev_cmd,0,2); >>>> + ff_ass_subtitle_header_default(avctx); >>> All the "= 0" and the memset are unnecessary. >> removed >>>> + return 0; >>>> +} >>>> + >>>> +static av_cold int close_decoder(AVCodecContext *avctx) >>>> +{ >>>> + CCaptionSubContext *ctx = avctx->priv_data; >>>> + av_bprint_finalize( &ctx->buffer, NULL); >>>> + return 0; >>>> +} >>>> + >>>> +/** >>>> + * This function after validating parity bit, also remove it from data >>>> pair. >>>> + */ >>>> +static int validate_cc_data_pair (unsigned char *cc_data_pair, int >>>> *parity_table) >>>> +{ >>>> + unsigned char cc_valid = (*cc_data_pair & 4) >>2; >>>> + unsigned char cc_type = *cc_data_pair & 3; >>>> + >>>> + if (!cc_valid) >>>> + return -1; >>>> + >>>> + // if EIA-608 data then verify parity. >>>> + if (cc_type==0 || cc_type==1) { >>>> + if (!parity_table[cc_data_pair[2]]) { >>>> + // If the second byte doesn't pass parity, ignore pair >>>> + return -1; >>> Meaningful error codes? AVERROR(EINVAL) or AVERROR_INVALID_DATA? Even when >>> they are not supposed to, the -1 have a habit of reaching the user. Same >>> below. >> done >>>> + } >>>> + if (!parity_table[cc_data_pair[1]]) { >>>> + // The first byte doesn't pass parity, we replace it with a solid >>>> blank >>>> + // and process the pair. >>>> + cc_data_pair[1]=0x7F; >>>> + } >>>> + } >>>> + >>>> + //Skip non-data >>>> + if( (cc_data_pair[0] == 0xFA || cc_data_pair[0] == 0xFC || >>>> cc_data_pair[0] == 0xFD ) >>>> + && (cc_data_pair[1] & 0x7F) == 0 && (cc_data_pair[2] & 0x7F) == >>>> 0) >>>> + return -1; >>>> + >>>> + //skip 708 data >>>> + if(cc_type == 3 || cc_type == 2 ) >>>> + return -1; >>>> + >>>> + /* remove parity bit */ >>>> + cc_data_pair[1] &= 0x7F; >>>> + cc_data_pair[2] &= 0x7F; >>>> + >>>> + >>>> + return 0; >>>> + >>>> +} >>>> +static void handle_pac( CCaptionSubContext *ctx, uint8_t hi, uint8_t lo ) >>>> +{ >>>> + static const int row_map[] = { >>> Could be int8_t to save memory. >> done >>>> + 11, -1, 1, 2, 3, 4, 12, 13, 14, 15, 5, 6, 7, 8, 9, 10 >>>> + }; >>>> + const int index = ( (hi<<1) & 0x0e) | ( (lo>>5) & 0x01 ); >>>> + >>> It looks like the values come from external data with subtle arithmetic, so >>> I suggest: >>> >>> av_assert2((unsigned)index < sizeof(row_map)); >> I am using 3 bit from hi and 1 bit from lo data, so I am using total 4 bit >> which mean only 0-15 possible. and for unknown value not defined in spec >> I have kept the value as -1 on that index. >> so no need to check. >>>> + if( row_map[index] <= 0 ) >>>> + return; >>>> + >>>> + ctx->cursor_row = row_map[index] - 1; >>>> + ctx->cursor_column = 0; >>>> + >>>> +} >>>> + >>>> +/** >>>> + * @param pts it is required to set end time >>>> + */ >>>> +static void handle_edm(CCaptionSubContext *ctx,int64_t pts) >>>> +{ >>>> + int i; >>>> + struct Screen *screen = ctx->screen + ctx->active_screen; >>>> + >>>> + ctx->start_time = ctx->startv_time; >>>> + for( i = 0; screen->row_used && i < SCREEN_ROWS; i++) >>>> + { >>>> + if(CHECK_FLAG(screen->row_used,i)) { >>>> + av_bprint_append_data(&ctx->buffer, screen->characters[i], >>>> strlen(screen->characters[i])); >>>> + av_bprint_append_data(&ctx->buffer, "\\N",2); >>>> + UNSET_FLAG(screen->row_used, i); >>>> + } >>>> + } >>>> + ctx->startv_time = pts; >>>> + ctx->edm = 1; >>>> + ctx->end_time = pts; >>>> +} >>>> +static void handle_eoc(CCaptionSubContext *ctx, int64_t pts) >>>> +{ >>>> + ctx->active_screen = !ctx->active_screen; >>>> + ctx->startv_time = pts; >>>> +} >>>> +static struct Screen *get_writing_screen(CCaptionSubContext *ctx) >>>> +{ >>>> + switch (ctx->mode) { >>>> + case CCMODE_POPON: >>>> + // use Inactive screen >>>> + return ctx->screen + !ctx->active_screen; >>>> + case CCMODE_PAINTON: >>>> + case CCMODE_ROLLUP_2: >>>> + case CCMODE_ROLLUP_3: >>>> + case CCMODE_ROLLUP_4: >>>> + case CCMODE_TEXT: >>>> + // use active screen >>>> + return ctx->screen + !ctx->active_screen; >>>> + } >>>> + /* It was never an option */ >>>> + return NULL; >>>> +} >>>> +static void handle_char(CCaptionSubContext *ctx, char hi, char lo, >>>> int64_t pts) >>>> +{ >>>> + struct Screen *screen = get_writing_screen(ctx); >>>> + char *row = screen->characters[ctx->cursor_row] + ctx->cursor_column; >>> Here too: av_assert0((unsigned)ctx->cursor_row < SCREEN_ROWS) and same for >>> column. >> We can never get row greater then SCREEN_ROWS, I have not implemented to >> read column value from pac. so left it as it is. >>>> + >>>> + SET_FLAG(screen->row_used,ctx->cursor_row); >>>> + >>>> + *row++ = hi; >>>> + ctx->cursor_column++; >>>> + if(lo) { >>>> + *row++ = lo; >>>> + ctx->cursor_column++; >>>> + } >>>> + *row = 0; >>>> + /* reset prev command since character can repeat */ >>>> + ctx->prev_cmd[0] = 0; >>>> + ctx->prev_cmd[1] = 0; >>>> +} >>>> +static int process_cc608(CCaptionSubContext *ctx, int64_t pts, unsigned >>>> char hi, unsigned char lo) >>>> +{ >>>> + >>>> +#define COR3(var, with1, with2, with3) ( (var) == (with1) || (var) == >>>> (with2) || (var) == (with3) ) >>> It looks like you always use COR3(hi, 0x14, 0x15, 0x1C) exactly: is it on >>> purpose, a typo, or maybe can you simplify? >> These are done for purpose, I have to implement some other command which >> have >> different values to be compared. so not simplifying it. >>>> + 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 */ >>>> + handle_edm(ctx, pts); >>>> + } else if ( COR3(hi, 0x14, 0x15, 0x1C) && lo == 0x2D ) { >>>> + /* carriage return */ >>>> + ctx->row_cnt++; >>>> + if(ctx->row_cnt == ctx->rollup) { >>>> + ctx->row_cnt = 0; >>>> + handle_edm(ctx, pts); >>>> + } >>>> + } else if ( COR3(hi, 0x14, 0x15, 0x1C) && lo == 0x2F ) { >>>> + /* end of caption */ >>>> + 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 0; >>>> + >>>> +} >>>> +static int decode(AVCodecContext *avctx, void *data, int *got_sub, >>>> AVPacket *avpkt) >>>> +{ >>>> + CCaptionSubContext *ctx = avctx->priv_data; >>>> + AVSubtitle *sub = data; >>>> + unsigned char *bptr = avpkt->data; >>>> + int len = avpkt->size; >>>> + int ret = 0; >>>> + int i; >>>> + >>>> + for (i = 0; i < len; i += 3) { >>>> + unsigned char 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->edm && *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 }); >>>> + ret = ff_ass_add_rect(sub, ctx->buffer.str, start_time, end_time >>>> - start_time , 0); >>>> a >>> You need to use av_bprint_is_complete() and return AVERROR(ENOMEM) if the >>> text was truncated. >> done in handle_edm >>>> + if (ret < 0) >>>> + return ret; >>>> + sub->pts = av_rescale_q(ctx->start_time, avctx->time_base, >>>> AV_TIME_BASE_Q); >>>> + ctx->edm = 0; >>>> + av_bprint_clear(&ctx->buffer); >>>> + } >>>> + >>>> + *got_sub = sub->num_rects > 0; >>>> + return 0; >>>> +} >>>> + >>>> +AVCodec ff_ccaption_decoder = { >>>> + .name = "cc_dec", >>>> + .long_name = NULL_IF_CONFIG_SMALL("Closed Caption Decoder"), >>> I suggest to have "EIA-608" and "CEA-708" appear in the name, for people >>> specifically looking for it: "Closed Caption (EIA-608 / CEA-708) Decoder" >>> maybe. >> done >>>> + .type = AVMEDIA_TYPE_SUBTITLE, >>>> + .id = AV_CODEC_ID_EIA_608, >>>> + .priv_data_size = sizeof(CCaptionSubContext), >>>> + .init = init_decoder, >>>> + .close = close_decoder, >>>> + .decode = decode, >>>> +}; >>> Regards, >>> >> I have also removed some error which were causing to print nothing in >> subccItalic.mpg. >> >> Though subtitle is still not italic. >> >> I was thinking to put extra features in next iteration of patch. >> >> -Anshul >> > Attaching patch with those work. ping
-please comment Anshul Maheshwari _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel