[FFmpeg-devel] [PATCH] dvbsub fix transcoding

[Anshul Maheshwari]

On Sat, Jun 21, 2014 at 12:13 AM, Michael Niedermayer <michaelni at gmx.at>
wrote:

> 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.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
I did those changes, but did attached an old patch getting confused because
of same name.
not yet made sure i dont set AVSubtitle twice, doing it.
attached patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-fix-transcoding-dvbsub-to-dvbsub.patch
Type: text/x-patch
Size: 5894 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140621/3774c7b2/attachment.bin>