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

[Michael Niedermayer]

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


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

The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- 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/20150106/0c424b47/attachment.asc>