[FFmpeg-devel] [PATCH] dvbsub fix transcoding
Michael Niedermayer
michaelni at gmx.at
Sat Jun 21 13:23:18 CEST 2014
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
[...]
> 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.
> ffmpeg.c | 6 +++++-
> libavcodec/dvbsubdec.c | 43 +++++++++++++++++++++++++++++++++----------
> 2 files changed, 38 insertions(+), 11 deletions(-)
> 33a3b2f616fe4914b2b238ebef8c4f6b7e0f9ce0 0001-fix-transcoding-dvbsub-to-dvbsub.patch
> From e5e06a37ca4fa9f0735b8992abbe30d9d260ea97 Mon Sep 17 00:00:00 2001
> From: Anshul Maheshwari <anshul.ffmpeg at gmail.com>
> Date: Sat, 21 Jun 2014 14:01:19 +0530
> Subject: [PATCH] fix transcoding dvbsub to dvbsub
>
> fix ticket #2024
> ---
> ffmpeg.c | 6 +++++-
> libavcodec/dvbsubdec.c | 43 +++++++++++++++++++++++++++++++++----------
> 2 files changed, 38 insertions(+), 11 deletions(-)
>
> diff --git a/ffmpeg.c b/ffmpeg.c
> index 1af3e0e..c5a2496 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -778,6 +778,7 @@ static void do_subtitle_out(AVFormatContext *s,
> AVCodecContext *enc;
> AVPacket pkt;
> int64_t pts;
> + unsigned save_rects = 0;
>
> if (sub->pts == AV_NOPTS_VALUE) {
> av_log(NULL, AV_LOG_ERROR, "Subtitle packets must have a pts\n");
> @@ -815,7 +816,7 @@ static void do_subtitle_out(AVFormatContext *s,
> sub->end_display_time -= sub->start_display_time;
> sub->start_display_time = 0;
> if (i == 1)
> - sub->num_rects = 0;
> + FFSWAP(int64_t,sub->num_rects,save_rects);
>
> ost->frames_encoded++;
>
> @@ -842,6 +843,7 @@ static void do_subtitle_out(AVFormatContext *s,
> pkt.dts = pkt.pts;
> write_frame(s, &pkt, ost);
> }
> + FFSWAP(int64_t,sub->num_rects,save_rects);
> }
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 ...
>
> 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++;
> }
> }
> + 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()
> +
> + return ret;
> }
>
> static void dvbsub_parse_pixel_data_block(AVCodecContext *avctx, DVBSubObjectDisplay *display,
> @@ -1227,7 +1240,7 @@ static void dvbsub_parse_region_segment(AVCodecContext *avctx,
> }
> }
>
> -static void dvbsub_parse_page_segment(AVCodecContext *avctx,
> +static int dvbsub_parse_page_segment(AVCodecContext *avctx,
> const uint8_t *buf, int buf_size, AVSubtitle *sub)
> {
> DVBSubContext *ctx = avctx->priv_data;
> @@ -1239,16 +1252,17 @@ static void dvbsub_parse_page_segment(AVCodecContext *avctx,
> int page_state;
> int timeout;
> int version;
> + int ret;
>
> if (buf_size < 1)
> - return;
> + return 0;
>
> timeout = *buf++;
> version = ((*buf)>>4) & 15;
> page_state = ((*buf++) >> 2) & 3;
>
> if (ctx->version == version) {
> - return;
> + return 0;
> }
>
> ctx->time_out = timeout;
> @@ -1256,6 +1270,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)
> + ret = save_subtitle_set(avctx,sub);
> +
> if (page_state == 1 || page_state == 2) {
> delete_regions(ctx);
> delete_objects(ctx);
> @@ -1302,6 +1319,7 @@ static void dvbsub_parse_page_segment(AVCodecContext *avctx,
>
> av_free(display);
> }
> + return ret;
>
returns a uninitialized variable in some cases
> }
>
> @@ -1449,13 +1467,15 @@ static int dvbsub_display_end_segment(AVCodecContext *avctx, const uint8_t *buf,
> int buf_size, AVSubtitle *sub)
> {
> DVBSubContext *ctx = avctx->priv_data;
> + int ret;
>
> - save_subtitle_set(avctx,sub);
> + if(ctx->compute_edt == 0)
> + ret = save_subtitle_set(avctx,sub);
> #ifdef DEBUG
> save_display_set(ctx);
> #endif
>
> - return 1;
> + return ret;
> }
returns a uninitialized variable in some cases
>
> 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 ?
[...]
--
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.
-------------- 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/7db03a1e/attachment.asc>
More information about the ffmpeg-devel
mailing list