[FFmpeg-devel] [PATCH] dvbsub fix transcoding
Anshul Maheshwari
anshul.ffmpeg at gmail.com
Sat Jun 21 16:41:02 CEST 2014
On Sat, Jun 21, 2014 at 4:53 PM, Michael Niedermayer <michaelni at gmx.at>
wrote:
> On Sat, Jun 21, 2014 at 02:06:38PM +0530, Anshul Maheshwari wrote:
> > On Sat, Jun 21, 2014 at 12:53 PM, Anshul Maheshwari <
> anshul.ffmpeg at gmail.com
> [...]
> when the loop executes once this is plain wrong and adds a memleak
> also half of what the swap does is write into a variable that is
> going out of scope
>
> and it should have been in a seperate patch, 1 patch == 1 issue
>
> either way, cleaned above up and applied, as this is just 3 lines
> not worth the work resubmiting for that ...
>
> Thanks
>
> >
> > static void do_video_out(AVFormatContext *s,
> > @@ -2277,6 +2279,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..90cc90e 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 time out */
> > + int64_t prev_start;
> > DVBSubRegion *region_list;
> > DVBSubCLUT *clut_list;
> > DVBSubObject *object_list;
> > @@ -383,6 +386,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;
> > @@ -759,7 +763,7 @@ static int dvbsub_read_8bit_string(uint8_t *destbuf,
> int dbuf_len,
> > return pixels_read;
> > }
> >
> > -static void save_subtitle_set(AVCodecContext *avctx, AVSubtitle *sub)
> > +static int save_subtitle_set(AVCodecContext *avctx, AVSubtitle *sub)
> > {
> > DVBSubContext *ctx = avctx->priv_data;
> > DVBSubRegionDisplay *display;
> > @@ -768,10 +772,11 @@ static void save_subtitle_set(AVCodecContext
> *avctx, AVSubtitle *sub)
> > AVSubtitleRect *rect;
> > DVBSubCLUT *clut;
> > uint32_t *clut_table;
> > - int i;
> > + int i,ret = 0;
> > 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 +791,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;
> > + ret = 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]));
> > @@ -837,6 +846,10 @@ static void save_subtitle_set(AVCodecContext
> *avctx, AVSubtitle *sub)
> > i++;
> > }ng t
> > }
> > + if(ctx->compute_edt == 1)
> > + FFSWAP(int64_t,ctx->prev_start,sub->pts);
>
> i dont think the swap will do what is intended if the code executes
> multiple times in a single call to dvbsub_decode()
>
> save_subtitle will be called once either in page_segment or in end_segment
according to option set
in single call to dvbsub_decode.
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,
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.
[...]
>
> --
> 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
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-fix-transcoding-dvbsub-to-dvbsub.patch
Type: text/x-patch
Size: 6693 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140621/70483fdc/attachment.bin>
More information about the ffmpeg-devel
mailing list