[FFmpeg-devel] [PATCH] dvbsub fix transcoding
Michael Niedermayer
michaelni at gmx.at
Sat Jun 21 19:47:55 CEST 2014
On Sat, Jun 21, 2014 at 08:11:02PM +0530, Anshul Maheshwari wrote:
> 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.
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
the multiple calls will also cause other problems like memleaks i
think
> 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
> 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.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- 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/20140621/2a1dbf0f/attachment.asc>
More information about the ffmpeg-devel
mailing list