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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
