[FFmpeg-devel] [PATCH] dvbsub fix transcoding
Michael Niedermayer
michaelni at gmx.at
Sun Jun 22 14:01:50 CEST 2014
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
>
> >
> > > 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
> + 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
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, ..."
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- 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/3ec6c49f/attachment.asc>
More information about the ffmpeg-devel
mailing list