[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