[FFmpeg-devel] [PATCH] dvbsub fix transcoding

[Michael Niedermayer]

On Fri, Jun 20, 2014 at 09:57:04PM +0530, Anshul Maheshwari wrote:
> On Fri, Jun 20, 2014 at 7:23 PM, Michael Niedermayer <michaelni at gmx.at>
> wrote:
> 
> > On Fri, Jun 20, 2014 at 04:06:51PM +0530, anshul wrote:
> > > On 06/16/2014 12:07 AM, Michael Niedermayer wrote:
> > > >diff --git a/libavcodec/dvbsubdec.c b/libavcodec/dvbsubdec.c
> > > >index e6e746d..c213524 100644
> > > >--- a/libavcodec/dvbsubdec.c
> > > >+++ b/libavcodec/dvbsubdec.c
> > > >@@ -234,6 +234,9 @@ typedef struct DVBSubContext {
> > > >
> > > >      int version;
> > > >      int time_out;
> > > >+    int compute_edt; /**< if 1 end display time calculated using pts
> > > >+                          if 0 (Default) calculated using pts */
> > > >"using pts" in both cases is wrong
> > > >
> > > It was a typo corrected.
> > > >>+    int64_t prev_start;
> > > >>      DVBSubRegion *region_list;
> > > >>      DVBSubCLUT   *clut_list;
> > > >>      DVBSubObject *object_list;
> > > >>@@ -771,7 +774,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 +790,8 @@ static void save_subtitle_set(AVCodecContext
> > *avctx, AVSubtitle *sub)
> > > >>      }
> > > >>
> > > >>      if (sub->num_rects > 0) {
> > > >>+        if(ctx->compute_edt == 1 && ctx->prev_start)
> > > >>
> > > >prev_start == 0 is the wrong thing to check for here, 0 shouldnt be
> > > >special, it could be a valid timestamp
> > > >
> > > changed it to AV_NOPTS
> > > >>+            sub->end_display_time = av_rescale_q((sub->pts -
> > ctx->prev_start )/90, AV_TIME_BASE_Q, avctx->time_base) - 1;
> > > >the /90 looks wrong as well
> > > >rescaling happen between 2 timebases, there should not be a 3rd factor
> > > >dividing the input before av_rescale_q()
> > > >
> > > done
> > > >>          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]));
> > > >>@@ -837,6 +843,8 @@ static void save_subtitle_set(AVCodecContext
> > *avctx, AVSubtitle *sub)
> > > >>              i++;
> > > >>          }
> > > >>      }
> > > >>+    if(ctx->compute_edt == 1)
> > > >>+        FFSWAP(int64_t,ctx->prev_start,sub->pts);
> > > >>  }
> > > >>
> > > >IIRC there can be "things" like for example differnent languages and
> > > >for example the begin of the english subtitle doesnt signify the end
> > > >of the previous german one
> > > >
> > > >also after this patch the "int *data_size," from the dvbsub_decode()
> > > >function will be wrongly set
> > > >
> > > for different language different dvbcontext are made, because we do
> > > not support
> > > or have code for different language with same elementry stream id.
> > > therefor no worry.
> >
> > >
> > > *data_size was not set after this patch, but I think it is not set
> > > correctly now or before
> >
> > data_size looks like it is set correctly before the patch but
> > incorrectly afterwards
> > data_size must be set to non zero when data is retured and 0 when not
> >
> > [...]
> >
> > --
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > Opposition brings concord. Out of discord comes the fairest harmony.
> > -- Heraclitus
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > new patch attached

data_size is still wrong

dvbsub_parse_page_segment() returns 1 at the end and sets data_size
yet it will only return a subtitle when compute_edt is set

or consider there are 2 page segments
the second call will overwrite the subtitles from the first, leaking
memory

similarly dvbsub_display_end_segment overwrites data_size again

i think a simple rule of thumb is when you set AVSubtitle, set
data_size to 1, otherwise do not touch it.
And make sure you dont set AVSubtitle twice

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/20140620/385c4f1d/attachment.asc>