[FFmpeg-devel] [PATCH 2/2] Adding closed caption decoder
Anshul Maheshwari
anshul.ffmpeg at gmail.com
Tue Jan 6 07:59:32 CET 2015
On Tue, Jan 6, 2015 at 5:47 AM, Michael Niedermayer <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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Adding-Closed-caption-Support.patch
Type: text/x-patch
Size: 20056 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150106/d8596444/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Adding-Closed-caption-accessories.patch
Type: text/x-patch
Size: 1632 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150106/d8596444/attachment-0001.bin>
More information about the ffmpeg-devel
mailing list