[FFmpeg-devel] [PATCH 2/2] Adding closed caption decoder

[Anshul]

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.

-Anshul
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Adding-Closed-caption-Support.patch
Type: text/x-patch
Size: 19833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150105/3eb9a862/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Adding-Closed-caption-accessories.patch
Type: text/x-patch
Size: 1634 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150105/3eb9a862/attachment-0001.bin>