On 01/06/2015 12:29 PM, Anshul Maheshwari wrote: > > > On Tue, Jan 6, 2015 at 5:47 AM, Michael Niedermayer <michae...@gmx.at > <mailto:michae...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
