[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