[FFmpeg-devel] [PATCH] dvbsub fix transcoding
Anshul Maheshwari
anshul.ffmpeg at gmail.com
Sun Jun 22 17:43:45 CEST 2014
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-fix-transcoding-dvbsub-to-dvbsub.patch
Type: text/x-patch
Size: 7580 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140622/9fb8272c/attachment.bin>
More information about the ffmpeg-devel
mailing list