[FFmpeg-devel] [PATCH] dvbsub fix transcoding
Michael Niedermayer
michaelni at gmx.at
Sun Jun 22 18:30:05 CEST 2014
On Sun, Jun 22, 2014 at 09:13:45PM +0530, Anshul Maheshwari wrote:
> On Sun, Jun 22, 2014 at 5:31 PM, Michael Niedermayer <michaelni at gmx.at>
> wrote:
>
> > On Sun, Jun 22, 2014 at 11:02:10AM +0530, Anshul Maheshwari wrote:
> > > On Sat, Jun 21, 2014 at 11:17 PM, Michael Niedermayer <michaelni at gmx.at>
> > > wrote:
> > >
> > > > On Sat, Jun 21, 2014 at 08:11:02PM +0530, Anshul Maheshwari wrote:
> > > > >
> > > > > > save_subtitle will be called once either in page_segment or in
> > > > end_segment
> > > > > according to option set
> > > > > in single call to dvbsub_decode.
> > > >
> > > > page_segment and end_segment can be called multiple times
> > > > in a single call to dvbsub_decode. This will cause save_subtitle_set()
> > > > to be called multiple times and will result in a nonsensical timestamp
> > > > with this FFSWAP() code
> > > >
> > > > done
> > >
> > > > the multiple calls will also cause other problems like memleaks i
> > > > think
> > > >
> > > > Will wait till someone give sample for this. I hope there would be
> > > something in spec
> > > ,some flag would be set when those function are called twice.
> >
> > well, adding a avpriv_request_sample() surely cant hurt, otherwise
> > its unlikely we will find such a sample soon
> >
> > still the code must not leak, even without a testcase being available
> > otherwise someone can create a file to cause any application to leak
> > to death, its not good if you click a link in your browser and
> > it crashes with OOM
>
>
> done
>
> >
> > >
> > > >
> > > > > I thought of adding one more security check by checking prev_start is
> > > > less
> > > > > then pts , but it will also fail if they are called
> > > > > when pts is up with its 33 bit and revolving back from 0.
> > > > > So i did moved that code in dvbsub_decode
> > > > >
> > > > >
> > > > > > returns a uninitialized variable in some cases
> > > > > >
> > > > > > initialized at both place
> > > > >
> > > > > >
> > > > > > >
> > > > > > > static int dvbsub_decode(AVCodecContext *avctx,
> > > > > > > @@ -1514,7 +1534,7 @@ static int dvbsub_decode(AVCodecContext
> > *avctx,
> > > > > > > ctx->composition_id == -1 || ctx->ancillary_id ==
> > -1) {
> > > > > > > switch (segment_type) {
> > > > > > > case DVBSUB_PAGE_SEGMENT:
> > > > > > > - dvbsub_parse_page_segment(avctx, p,
> > segment_length,
> > > > > > sub);
> > > > > > > + ret = dvbsub_parse_page_segment(avctx, p,
> > > > > > segment_length, sub);
> > > > > > > got_segment |= 1;
> > > > > > > break;
> > > > > > > case DVBSUB_REGION_SEGMENT:
> > > > > > > @@ -1534,7 +1554,7 @@ static int dvbsub_decode(AVCodecContext
> > *avctx,
> > > > > > > dvbsub_parse_display_definition_segment(avctx,
> > p,
> > > > > > segment_length);
> > > > > > > break;
> > > > > > > case DVBSUB_DISPLAY_SEGMENT:
> > > > > > > - *data_size = dvbsub_display_end_segment(avctx,
> > p,
> > > > > > segment_length, sub);
> > > > > > > + ret = dvbsub_display_end_segment(avctx, p,
> > > > > > segment_length, sub);
> > > > > > > got_segment |= 16;
> > > > > > > break;
> > > > > > > default:
> > > > > > > @@ -1543,6 +1563,8 @@ static int dvbsub_decode(AVCodecContext
> > *avctx,
> > > > > > > break;
> > > > > > > }
> > > > > > > }
> > > > > > > + if(ret > *data_size)
> > > > > > > + *data_size = ret;
> > > > > >
> > > > > > the code now is a bit more complex but it does not look more
> > > > > > correct to me.
> > > > > >
> > > > > > can you explain why you set data_size not simply when you
> > > > > > set the subtitle and leave it alone when you do not ?
> > > > > >
> > > > >
> > > > > In dvbsub_display_end_segment function save_display_set function is
> > not
> > > > at
> > > > > all called,
> > > >
> > > > noone talked about save_display_set() which is under #ifdef DEBUG
> > > >
> > > >
> > > >
> > > ohh, I was saying about save_subtitle_set()
> > >
> > > > > and i don't get to know whether subtitle is set there or not.
> > > > > checking sub->num_rects in dvbsub_display_end_segment and setting
> > return
> > > > > acc to that.
> > > > > does not look good to me.
> > > > > If you want me to check that i will do that way.
> > > >
> > > > save_subtitle_set() sets AVSubtitle
> > > > save_subtitle_set() should also set data_size when it sets up
> > > > AVSubtitle, that is to indicate that AVSubtitle has been set
> > > > no other code should set data_size, that is unless there is other
> > > > code that sets AVSubtitle
> > > >
> > > > data_size instead could be set outside but then it should be
> > > > immedeatly vissible that they match and are correct.
> > > > Not like these patches that set data_size and AVSubtitle at different
> > > > points in different functions in a loop and are wrong on closer
> > > > inspection.
> > > >
> > >
> > > tried
> > >
> > > New patch attached
> >
> > > ffmpeg.c | 2 ++
> > > libavcodec/dvbsubdec.c | 30 ++++++++++++++++++++++++------
> > > 2 files changed, 26 insertions(+), 6 deletions(-)
> > > d956df37c653485fd300979466953e1b3a7ad984
> > 0001-fix-transcoding-dvbsub-to-dvbsub.patch
> > > From 85169e630350e80466707bbd6bdb67744ae279b8 Mon Sep 17 00:00:00 2001
> > > From: Anshul Maheshwari <anshul.ffmpeg at gmail.com>
> > > Date: Sun, 22 Jun 2014 00:12:56 +0530
> > > Subject: [PATCH] fix transcoding dvbsub to dvbsub
> > >
> > > fix ticket #2024
> > > ---
> > > ffmpeg.c | 2 ++
> > > libavcodec/dvbsubdec.c | 30 ++++++++++++++++++++++++------
> > > 2 files changed, 26 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/ffmpeg.c b/ffmpeg.c
> > > index 9d9c4f4..91e4734 100644
> > > --- a/ffmpeg.c
> > > +++ b/ffmpeg.c
> > > @@ -2281,6 +2281,8 @@ static int init_input_stream(int ist_index, char
> > *error, int error_len)
> > > ist->dec_ctx->thread_safe_callbacks = 1;
> > >
> > > av_opt_set_int(ist->dec_ctx, "refcounted_frames", 1, 0);
> > > + if(ist->dec_ctx->codec_id == AV_CODEC_ID_DVB_SUBTITLE)
> > > + av_dict_set(&ist->decoder_opts, "compute_edt", "1", 0);
> > >
> > > if (!av_dict_get(ist->decoder_opts, "threads", NULL, 0))
> > > av_dict_set(&ist->decoder_opts, "threads", "auto", 0);
> > > diff --git a/libavcodec/dvbsubdec.c b/libavcodec/dvbsubdec.c
> > > index e6e746d..eadfef7 100644
> > > --- a/libavcodec/dvbsubdec.c
> > > +++ b/libavcodec/dvbsubdec.c
> > > @@ -234,6 +234,10 @@ typedef struct DVBSubContext {
> > >
> > > int version;
> > > int time_out;
> > > + int got_output : 1;
> >
> > > + int compute_edt : 1; /**< if 1 end display time calculated using pts
> > > + if 0 (Default) calculated using time out */
> >
> > doesnt work with AVOption, its not a plain int for FF_OPT_TYPE_INT
> >
> >
> > Done
>
> > > + int64_t prev_start;
> > > DVBSubRegion *region_list;
> > > DVBSubCLUT *clut_list;
> > > DVBSubObject *object_list;
> > > @@ -383,6 +387,7 @@ static av_cold int
> > dvbsub_init_decoder(AVCodecContext *avctx)
> > > }
> > >
> > > ctx->version = -1;
> > > + ctx->prev_start = AV_NOPTS_VALUE;
> > >
> > > default_clut.id = -1;
> > > default_clut.next = NULL;
> > > @@ -771,7 +776,8 @@ static void save_subtitle_set(AVCodecContext *avctx,
> > AVSubtitle *sub)
> > > int i;
> > > int offset_x=0, offset_y=0;
> > >
> > > - sub->end_display_time = ctx->time_out * 1000;
> > > + if(ctx->compute_edt == 0)
> > > + sub->end_display_time = ctx->time_out * 1000;
> > >
> > > if (display_def) {
> > > offset_x = display_def->x;
> > > @@ -786,6 +792,10 @@ static void save_subtitle_set(AVCodecContext
> > *avctx, AVSubtitle *sub)
> > > }
> > >
> > > if (sub->num_rects > 0) {
> > > + if(ctx->compute_edt == 1 && ctx->prev_start != AV_NOPTS_VALUE) {
> > > + sub->end_display_time = av_rescale_q((sub->pts -
> > ctx->prev_start ),AV_TIME_BASE_Q,(AVRational){ 1, 1000 }) - 1;
> > > + ctx->got_output = 1;
> > > + }
> > > sub->rects = av_mallocz_array(sizeof(*sub->rects),
> > sub->num_rects);
> > > for(i=0; i<sub->num_rects; i++)
> > > sub->rects[i] = av_mallocz(sizeof(*sub->rects[i]));
> > > @@ -1256,6 +1266,9 @@ static void
> > dvbsub_parse_page_segment(AVCodecContext *avctx,
> > >
> > > av_dlog(avctx, "Page time out %ds, state %d\n", ctx->time_out,
> > page_state);
> > >
> > > + if(ctx->compute_edt == 1)
> > > + save_subtitle_set(avctx,sub);
> > > +
> > > if (page_state == 1 || page_state == 2) {
> > > delete_regions(ctx);
> > > delete_objects(ctx);
> > > @@ -1445,17 +1458,17 @@ static void
> > dvbsub_parse_display_definition_segment(AVCodecContext *avctx,
> > > }
> > > }
> > >
> > > -static int dvbsub_display_end_segment(AVCodecContext *avctx, const
> > uint8_t *buf,
> > > +static void dvbsub_display_end_segment(AVCodecContext *avctx, const
> > uint8_t *buf,
> > > int buf_size, AVSubtitle *sub)
> > > {
> > > DVBSubContext *ctx = avctx->priv_data;
> > >
> > > - save_subtitle_set(avctx,sub);
> > > + if(ctx->compute_edt == 0)
> > > + save_subtitle_set(avctx,sub);
> > > #ifdef DEBUG
> > > save_display_set(ctx);
> > > #endif
> > >
> > > - return 1;
> > > }
> > >
> > > static int dvbsub_decode(AVCodecContext *avctx,
> > > @@ -1534,7 +1547,7 @@ static int dvbsub_decode(AVCodecContext *avctx,
> > > dvbsub_parse_display_definition_segment(avctx, p,
> > segment_length);
> > > break;
> > > case DVBSUB_DISPLAY_SEGMENT:
> > > - *data_size = dvbsub_display_end_segment(avctx, p,
> > segment_length, sub);
> > > + dvbsub_display_end_segment(avctx, p, segment_length,
> > sub);
> > > got_segment |= 16;
> > > break;
> > > default:
> >
> > > @@ -1550,13 +1563,18 @@ static int dvbsub_decode(AVCodecContext *avctx,
> > > // segments then we need no further data.
> > > if (got_segment == 15 && sub) {
> > > av_log(avctx, AV_LOG_DEBUG, "Missing display_end_segment,
> > emulating\n");
> > > - *data_size = dvbsub_display_end_segment(avctx, p, 0, sub);
> > > + dvbsub_display_end_segment(avctx, p, 0, sub);
> > > }
> > > + *data_size = ctx->got_output;
> > > + ctx->got_output = 0;
> >
> > thats still broken
> >
> > you reset ctx->got_output only at the end, this implies that it will
> > still be set in case of a return due to error and be wrong on the next
> > call, also iam not sure its otherwise set correctly
> >
> > Also i think you should not treat AVSubtitle and data_size differently
> > they should be set together, never one without the other
> >
> > also their lifetime differs from the context lifetime which makes it
> > confusing (and error prone) to put them in the context IMHO
> >
> > Done
>
> > please review and think about your code before submitting a patch
> > that is dont just ask yourself "does this work" because its easy
> > to fool yourself then
> > ask yourself "how can i make it fail", "with what input will it crash,
> > leak memory, set data_size incorrectly, ..."
> >
> > Thanks
>
> Sorry for wasting your time,I hope this time all goes well.
> -Anshul
[...]
> @@ -786,6 +795,10 @@ static void save_subtitle_set(AVCodecContext *avctx, AVSubtitle *sub)
> }
>
> if (sub->num_rects > 0) {
> + if(ctx->compute_edt == 1 && ctx->prev_start != AV_NOPTS_VALUE) {
> + sub->end_display_time = av_rescale_q((sub->pts - ctx->prev_start ), AV_TIME_BASE_Q, (AVRational){ 1, 1000 }) - 1;
> + *got_output = 1;
> + }
this seems to be the only place that sets got_output to 1
and its only executed when compute_edt == 1
so compute_edt == 0 would never return anything i suspect
[...]
> + if(ret < 0) {
> + *data_size = 0;
> + avsubtitle_free(sub);
> + return -1;
iam not sure this is neccesary
but ret should be passed through and not replaced by -1 at least
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates
-------------- 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/20140622/513291c3/attachment.asc>
More information about the ffmpeg-devel
mailing list