[FFmpeg-devel] [PATCH] lavc/srtenc: use bprint for text buffers.

[Clément Bœsch]

On Mon, Nov 25, 2013 at 04:34:10PM +0100, Nicolas George wrote:
> Fix trac ticket #3120.
> 
> Signed-off-by: Nicolas George <george at nsup.org>
> ---
>  libavcodec/srtenc.c | 49 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/libavcodec/srtenc.c b/libavcodec/srtenc.c
> index 3036a7f..461399a 100644
> --- a/libavcodec/srtenc.c
> +++ b/libavcodec/srtenc.c
> @@ -22,6 +22,7 @@
>  #include <stdarg.h>
>  #include "avcodec.h"
>  #include "libavutil/avstring.h"
> +#include "libavutil/bprint.h"
>  #include "ass_split.h"
>  #include "ass.h"
>  
> @@ -31,10 +32,8 @@
>  typedef struct {
>      AVCodecContext *avctx;
>      ASSSplitContext *ass_ctx;
> -    char buffer[2048];
> -    char *ptr;
> -    char *end;
> -    char *dialog_start;
> +    AVBPrint buffer;
> +    unsigned timestamp_end;
>      int count;
>      char stack[SRT_STACK_SIZE];
>      int stack_ptr;
> @@ -49,7 +48,7 @@ static void srt_print(SRTContext *s, const char *str, ...)
>  {
>      va_list vargs;
>      va_start(vargs, str);
> -    s->ptr += vsnprintf(s->ptr, s->end - s->ptr, str, vargs);
> +    av_vbprintf(&s->buffer, str, vargs);
>      va_end(vargs);
>  }
>  
> @@ -144,8 +143,7 @@ static av_cold int srt_encode_init(AVCodecContext *avctx)
>  static void srt_text_cb(void *priv, const char *text, int len)
>  {
>      SRTContext *s = priv;
> -    av_strlcpy(s->ptr, text, FFMIN(s->end-s->ptr, len+1));
> -    s->ptr += len;
> +    av_bprint_append_data(&s->buffer, text, len);
>  }
>  

>  static void srt_new_line_cb(void *priv, int forced)
> @@ -208,11 +206,19 @@ static void srt_move_cb(void *priv, int x1, int y1, int x2, int y2,
>      char buffer[32];
>      int len = snprintf(buffer, sizeof(buffer),
>                         "  X1:%03u X2:%03u Y1:%03u Y2:%03u", x1, x2, y1, y2);
> -    if (s->end - s->ptr > len) {
> -        memmove(s->dialog_start+len, s->dialog_start, s->ptr-s->dialog_start+1);
> -        memcpy(s->dialog_start, buffer, len);
> -        s->ptr += len;
> +    unsigned char *dummy;
> +    unsigned room;
> +
> +    av_bprint_get_buffer(&s->buffer, len, &dummy, &room);
> +    if (room >= len) {
> +        memmove(s->buffer.str + s->timestamp_end + len,
> +                s->buffer.str + s->timestamp_end,
> +                s->buffer.len - s->timestamp_end + 1);
> +        memcpy(s->buffer.str + s->timestamp_end, buffer, len);
>      }
> +    /* Increment even if av_bprint_get_buffer() did not return enough room:
> +       the bprint structure will be treated as truncated. */
> +    s->buffer.len += len;
>      }

Make sure this is tested, it looks sensible, and I can't tell if this is
correct just from looking (and it's not covered by FATE according to
http://coverage.ffmpeg.org/ffmpeg/libavcodec/srtenc.c.gcov.html)

>  }
>  
> @@ -243,10 +249,11 @@ static int srt_encode_frame(AVCodecContext *avctx,
>  {
>      SRTContext *s = avctx->priv_data;
>      ASSDialog *dialog;
> -    int i, len, num;
> +    int i, num;
>  
> -    s->ptr = s->buffer;
> -    s->end = s->ptr + sizeof(s->buffer);

> +    if (!s->buffer.str)
> +        av_bprint_init(&s->buffer, 0, AV_BPRINT_SIZE_UNLIMITED);

This belongs in srt_encode_init().

> +    av_bprint_clear(&s->buffer);
>  
>      for (i=0; i<sub->num_rects; i++) {
>  
> @@ -268,7 +275,7 @@ static int srt_encode_frame(AVCodecContext *avctx,
>                  es = ec/   1000;  ec -=    1000*es;
>                  srt_print(s,"%d\r\n%02d:%02d:%02d,%03d --> %02d:%02d:%02d,%03d\r\n",
>                            ++s->count, sh, sm, ss, sc, eh, em, es, ec);
> -                s->dialog_start = s->ptr - 2;
> +                s->timestamp_end = s->buffer.len - 2;
>              }
>              s->alignment_applied = 0;
>              srt_style_apply(s, dialog->style);
> @@ -276,23 +283,25 @@ static int srt_encode_frame(AVCodecContext *avctx,
>          }
>      }
>  
> -    if (s->ptr == s->buffer)
> +    if (!av_bprint_is_complete(&s->buffer))
> +        return AVERROR(ENOMEM);
> +    if (!s->buffer.len)
>          return 0;
>  

> -    len = av_strlcpy(buf, s->buffer, bufsize);
> -
> -    if (len > bufsize-1) {
> +    if (s->buffer.len > bufsize) {
>          av_log(avctx, AV_LOG_ERROR, "Buffer too small for ASS event.\n");
>          return -1;
>      }
> +    memcpy(buf, s->buffer.str, s->buffer.len);

I never remember; isn't len supposed to not count the trailing 0? If so, I
think it should keep the bufsize-1 and copy the last 0 byte as well.

>  
> -    return len;
> +    return s->buffer.len;
>  }
>  
>  static int srt_encode_close(AVCodecContext *avctx)
>  {
>      SRTContext *s = avctx->priv_data;
>      ff_ass_split_free(s->ass_ctx);
> +    av_bprint_finalize(&s->buffer, NULL);
>      return 0;
>  }
>  

Should be OK otherwise, thanks for looking at this.

Note: this is cool, and we definitely need to make the new subtitles
encoding API using a codec-allocated buffer of some sort.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131126/747ab5a7/attachment.asc>