[FFmpeg-devel] [PATCH 2/2] Adding closed caption decoder
Michael Niedermayer
michaelni at gmx.at
Sun Jan 4 17:47:20 CET 2015
On Sun, Jan 04, 2015 at 08:21:51PM +0530, Anshul wrote:
> On 01/03/2015 08:40 PM, Michael Niedermayer wrote:
> > On Sat, Jan 03, 2015 at 12:57:04PM +0530, Anshul wrote:
> >> On 01/03/2015 01:42 AM, Michael Niedermayer wrote:
> >>> On Wed, Dec 31, 2014 at 07:09:33PM +0530, Anshul wrote:
> > [..]
> >> Makefile | 1
> >> allcodecs.c | 1
> >> ccaption_dec.c | 361 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 363 insertions(+)
> >> 54d4896ef8724994e1022eec6a9c79d0cddec29d 0001-Adding-Closed-caption-Support.patch
> >> From 17a564409b84fc18293833cc3f2151792209bb8b Mon Sep 17 00:00:00 2001
> >> From: Anshul Maheshwari <anshul.ffmpeg at gmail.com>
> >> Date: Sat, 3 Jan 2015 12:40:35 +0530
> >> Subject: [PATCH 1/2] Adding Closed caption Support
> >>
> >> Signed-off-by: Anshul Maheshwari <anshul.ffmpeg at gmail.com>
> >>
> >> To test Closed caption use following command
> >> /ffmpeg -f lavfi -i "movie=/home/a141982112/test_videos/Starship_Troopers.vob[out0+subcc]" -map s some.srt
> >> ---
> >> libavcodec/Makefile | 1 +
> >> libavcodec/allcodecs.c | 1 +
> >> libavcodec/ccaption_dec.c | 361 ++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 363 insertions(+)
> >> create mode 100644 libavcodec/ccaption_dec.c
> >>
> >> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> >> index 107661b..33051c4 100644
> >> --- a/libavcodec/Makefile
> >> +++ b/libavcodec/Makefile
> >> @@ -173,6 +173,7 @@ OBJS-$(CONFIG_BRENDER_PIX_DECODER) += brenderpix.o
> >> OBJS-$(CONFIG_C93_DECODER) += c93.o
> >> OBJS-$(CONFIG_CAVS_DECODER) += cavs.o cavsdec.o cavsdsp.o \
> >> cavsdata.o mpeg12data.o
> >> +OBJS-$(CONFIG_CCAPTION_DECODER) += ccaption_dec.o
> >> OBJS-$(CONFIG_CDGRAPHICS_DECODER) += cdgraphics.o
> >> OBJS-$(CONFIG_CDXL_DECODER) += cdxl.o
> >> OBJS-$(CONFIG_CINEPAK_DECODER) += cinepak.o
> >> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> >> index 8ceee2f..ef77dec 100644
> >> --- a/libavcodec/allcodecs.c
> >> +++ b/libavcodec/allcodecs.c
> >> @@ -481,6 +481,7 @@ void avcodec_register_all(void)
> >> /* subtitles */
> >> REGISTER_ENCDEC (SSA, ssa);
> >> REGISTER_ENCDEC (ASS, ass);
> >> + REGISTER_DECODER(CCAPTION, ccaption);
> >> REGISTER_ENCDEC (DVBSUB, dvbsub);
> >> REGISTER_ENCDEC (DVDSUB, dvdsub);
> >> REGISTER_DECODER(JACOSUB, jacosub);
> >> diff --git a/libavcodec/ccaption_dec.c b/libavcodec/ccaption_dec.c
> >> new file mode 100644
> >> index 0000000..d351efe
> >> --- /dev/null
> >> +++ b/libavcodec/ccaption_dec.c
> >> @@ -0,0 +1,361 @@
> >> +/*
> >> + * Closed Caption Decoding
> >> + * Copyright (c) 2014 Anshul Maheshwari
> >> + *
> >> + * This file is part of FFmpeg.
> >> + *
> >> + * FFmpeg is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU Lesser General Public
> >> + * License as published by the Free Software Foundation; either
> >> + * version 2.1 of the License, or (at your option) any later version.
> >> + *
> >> + * FFmpeg is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> >> + * Lesser General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU Lesser General Public
> >> + * License along with FFmpeg; if not, write to the Free Software
> >> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> >> + */
> >> +
> >> +#include "avcodec.h"
> >> +#include "ass.h"
> >> +#include "libavutil/opt.h"
> >> +
> >> +#undef CHAR_DEBUG
> >> +#define SCREEN_ROWS 15
> >> +#define SCREEN_COLUMNS 32
> >> +
> >> +#define SET_FLAG(var, val) ( var |= ( 1 << (val) ) )
> >> +#define UNSET_FLAG(var, val) ( var &= ~( 1 << (val)) )
> >> +#define CHECK_FLAG(var, val) ( (var) & (1 << (val) ) )
> >> +
> >> +enum cc_mode {
> >> + CCMODE_POPON,
> >> + CCMODE_PAINTON,
> >> + CCMODE_ROLLUP_2,
> >> + CCMODE_ROLLUP_3,
> >> + CCMODE_ROLLUP_4,
> >> + CCMODE_TEXT,
> >> +};
> >> +
> >> +struct Screen {
> >> + /* +1 is used to compensate null character of string */
> >> + uint8_t characters[SCREEN_ROWS][SCREEN_COLUMNS+1];
> >> + /*
> >> + * Bitmask of used rows; if a bit is not set, the
> >> + * corresponding row is not used.
> >> + * for setting row 1 use row | (0 << 1)
> >> + * for setting row 15 use row | (1 << 14)
> >> + */
> >> + int16_t row_used;
> > you can use an array here, this would simplify the code and also
> > avoid the *_FLAG macros
> >
> >
> to check whether any row is used or not, It will have for loop for 15 rows,
> now row_used can be used directly in if and for loop to see whether any
> row is used or not.
>
> In ccextractor we use array for it, but its more complicated.
> This version of closed caption decoder is not full fledged, we will need
> to use row_used in many more commands.
ok
> >> +};
> >> +
> >> +
> >> +typedef struct CCaptionSubContext {
> >> + AVClass *class;
> >> + int parity_table[256];
> > this can be a static uint8_t table
> >
> I don't think static variable in structure are allowed in c language
> that is cpp thing.
>
> If you meant to remove that table from structure, then too its
> not efficient, we have to make parity table every time decode
> function is called.
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] = {...}
> >> + 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)
>
> >> +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
[...]
> +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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- 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/20150104/a9b19020/attachment.asc>
More information about the ffmpeg-devel
mailing list