On Mon, Jan 12, 2015 at 01:11:21AM +0530, Anshul wrote: > 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?
no, there where some comments on IRC but IIRC they where along the lines of future improvments not objections/blocking so applied Thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Avoid a single point of failure, be that a person or equipment.
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
