[FFmpeg-devel] [PATCH] dvbsub fix transcoding

[Anshul Maheshwari]

On Sat, Jun 21, 2014 at 12:53 PM, Anshul Maheshwari <anshul.ffmpeg at gmail.com
> wrote:

>
>
>
> 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
>

verified using valgrind now no memory leakage introduced because of this
patch on my pc.
also corrected making num_rects = 0 to send dummy frame to dvb encoder.

attached new patch.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-fix-transcoding-dvbsub-to-dvbsub.patch
Type: text/x-patch
Size: 7416 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140621/93b18db6/attachment.bin>