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

[Anshul]

On 01/06/2015 12:29 PM, Anshul Maheshwari wrote:
>
>
> On Tue, Jan 6, 2015 at 5:47 AM, Michael Niedermayer <michaelni at gmx.at
> <mailto:michaelni at gmx.at>> wrote:
>
>     On Mon, Jan 05, 2015 at 06:20:15PM +0530, Anshul wrote:
>     > On 01/04/2015 10:17 PM, Michael Niedermayer wrote:
>     > > the table is constant and does not change, theres no need to have
>     > > a copy of it in each context or to "make it every time decode is
>     > > called"
>     > > a simple static uint8_t parity_table[256];
>     > > or even
>     > > static const uint8_t parity_table[256] = {...}
>     > >
>     > done
>     > >>>> +    int row_cnt;
>     > >>>> +    struct Screen screen[2];
>     > >>>> +    int active_screen;
>     > >>>> +    uint8_t cursor_row;
>     > >>>> +    uint8_t cursor_column;
>     > >>>> +    AVBPrint buffer;
>     > >>>> +    int erase_display_memory;
>     > >>>> +    int rollup;
>     > >>>> +    enum  cc_mode mode;
>     > >>>> +    int64_t start_time;
>     > >>>> +    /* visible screen time */
>     > >>>> +    int64_t startv_time;
>     > >>>> +    int64_t end_time;
>     > >>>> +    char prev_cmd[2];
>     > >>>> +    /* buffer to store pkt data */
>     > >>>> +    AVBufferRef *pktbuf;
>     > >>> as you memcopy the data each time, theres no need for a
>     AVBufferRef
>     > >>> a simple uint8_t * would do the same
>     > >>> but i think not even that is needed,
>     > >>> all uses of the data go through process_cc608() it would be
>     > >>> very simply to strip one bit in the arguments there, so no
>     rewriting
>     > >>> of the table would be needed
>     > >>>
>     > >>> [...]
>     > >> cant do that, for error resistance we need to escape 1st byte
>     > >> if parity does not match, for escaping I write 0x7f instead of
>     > >> whatever data is. Some closed caption insert-er don't care
>     much for parity
>     > >> when they are not using the data.
>     > >>
>     > >> I was using AVBufferRef instead of uint8_t * , so that I
>     don't have to
>     > >> take care for length,
>     > >> and length and data are in one context. and there is already
>     lot of
>     > >> error handling is
>     > >> done while realloc, means I don't have to copy buffer pointer
>     somewhere,
>     > >> if realloc fails.
>     > >> and in future if someone want to make data channel 1  and
>     data channel 2
>     > >> to be processed
>     > >> in parallel,  then he may use reference thing too.
>     > >> still its my opinion, if you think uint8_t will have better
>     performance,
>     > >> I will change it to that.
>     > > if you prefer AVBufferRef, feel free to keep using it, i dont
>     think
>     > > its the optimal choice though.
>     > >
>     > > Also isnt there some maximum size for these buffers ?
>     > > (this would allow using a fixed size buffer and avoid the need for
>     > >  dealing with allocation failures)
>     > >
>     > There is nothing similar inside spec cc608. since its line 21 data,
>     > there must
>     > be something in vertical ancillary specification. I have to
>     search for
>     > that spec.
>     > if you can find about max limit of vanc packet, then ccaption
>     can not
>     > exceed
>     > with that.
>     > >>>> +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;
>     > >>>> +
>     > >>>> +    SET_FLAG(screen->row_used,ctx->cursor_row);
>     > >>>> +
>     > >>>> +    *row++ = hi;
>     > >>>> +    ctx->cursor_column++;
>     > >>>> +    if(lo) {
>     > >>>> +        *row++ = lo;
>     > >>>> +        ctx->cursor_column++;
>     > >>>> +    }
>     > >>>> +    *row = 0;
>     > >>> this code appears to lack validity checks on the column index
>     > >> Added in todo list, will do it while implementing backspace.
>     > > out of array accesses are not a "todo for later" they are a
>     > > critical issue that could allow an attacker to potentially execute
>     > > arbitrary code, that has to be fixed before the patch can be
>     applied
>     > done, took you comment wrongly.
>     > >
>     > > [...]
>     > >
>     > >
>     > >
>     > >> +static int decode(AVCodecContext *avctx, void *data, int
>     *got_sub, AVPacket *avpkt)
>     > >> +{
>     > >> +    CCaptionSubContext *ctx = avctx->priv_data;
>     > >> +    AVSubtitle *sub = data;
>     > >> +    uint8_t *bptr = NULL;
>     > >> +    int len = avpkt->size;
>     > >> +    int ret = 0;
>     > >> +    int i;
>     > >> +
>     > >> +    if ( ctx->pktbuf->size < len) {
>     > >> +        ret = av_buffer_realloc(&ctx->pktbuf, len);
>     > >> +        if(ret)
>     > >> +            len = ctx->pktbuf->size;
>     > >> +    }
>     > > error checks in ffmpeg  are <0 not != 0
>     > > also i doubt that it makes sense to continue with a truncated
>     packet
>     > > and if the code does continue with a data buffer that failed to
>     > > reallocate that would at least need an error/warning message
>     so the
>     > > user knows why what she sees is corrupted
>     > done
>     >
>     > attached new patchs, for column number, I have done lots of changes,
>     > you might want to reread the patch.
>
>     [...]
>
>     > +/**
>     > + * @param ctx closed caption context just to print log
>     > + */
>     > +static void write_char (CCaptionSubContext *ctx, char *row, int
>     col, char ch)
>     > +{
>     > +    if(col < SCREEN_COLUMNS)
>     > +        row[col] = ch;
>     > +    /* We have extra space at end only for null character */
>     > +    else if ( col == SCREEN_COLUMNS && ch == 0)
>     > +        row[col] = ch;
>     > +    else
>     > +        av_log(ctx, AV_LOG_WARNING,"Data Ignored since exciding
>     screen width\n");
>     > +}
>
>     The else implies that cursor_column can be an index outside the array
>     handle_delete_end_of_row() uses it as index without check
>     also is there something that prevents cursor_column from overflowing
>     and becoming negative ? such overflow would be undefined behavior
>     for a signed variable and also failing the checks above, but maybe
>     iam just missing somethig and it cannot overflow
>
> done,
>
> it cant be negative now,
> changed write_char function parameter to uint8_t same as cursor_column.
>
> for more error resistance
> cursor_column is increased only if there is space to increase means
> only if
> write_char was successful.
>
> now handle_delete_end_of_row() is using write_char()
> new patch attached
>
> -Anshul
anything else required from my side?

-Anshul