[FFmpeg-devel] [PATCH 2/2] Adding closed caption decoder
Michael Niedermayer
michaelni at gmx.at
Sun Jan 11 22:36:07 CET 2015
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 <michaelni at gmx.at
> > <mailto: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
> 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.
-------------- 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/20150111/e80221d0/attachment.asc>
More information about the ffmpeg-devel
mailing list