[FFmpeg-devel] [RFC] AVSubtitles rework

Clément Bœsch ubitux at gmail.com
Mon Sep 3 00:59:39 CEST 2012


On Sun, Sep 02, 2012 at 09:31:01PM +0200, Nicolas George wrote:
> Le septidi 17 fructidor, an CCXX, Clément Bœsch a écrit :
> > After speaking a little with Nicolas & Alexander yesterday, it appeared
> > that introducing avcodec_get_subtitle_defaults()/avcodec_alloc_subtitle()
> > to avoid ABI problems with sizeof(AVSubtitle) would be the first step in
> > improving the current subtitles situation.
> 
> I wonder if it is necessary to make it a new structure. That would force
> people to realize that something has changed, and allow to add new fields at
> the end without touching the original structure.
> 
> If we decide for a new structure, I suggest to have the first field "int
> struct_size", set to sizeof(struct) by the alloc function. That makes an
> easy and reliable consistency test.
> 

I'm not very fond of introducing a new structure for a few reasons:
 - having a AVSubtitle2 will require to maintain both paths for longer,
   and the problem is already hard to deal with even if starting from
   scratch
 - if we do that, it will require duplicating the current public API for a
   while, which sounds kind of a pain
 - I don't think the current AVSubtitle API is really used apart from
   MPlayer, but I may be wrong

I believe fixing that struct allocation is the correct choice to allow use
to smoothly move ahead without breaking anything later. It's likely we
won't add stuff in AVSubtitle for a while anyway, since we first need to
decide how to store the styles, and the events+styles.

That said, if people prefer writing a completely new API on top of the
current one instead and maintain the both, then be it. Though, I don't
feel much motivated to do it myself…

Attached to this mail the patches for the attempt to introduce & use
avcodec_alloc_subtitle(). The patchset is assuming a minor bump in lavc.

> > Do we want to move the AVSubtitleRect from AVSubtitles to the codec
> > context (and keep a pointer in the AVSubtitleRect)?
> 
> Unless I am mistaken, the only reason to have the frame data belong to the
> codec context rather than the frame is to allow references frames and direct
> rendering using the get_buffer/release_buffer logic. I do not think we need
> that kind of logic for subtitles, but I may be wrong.
> 
> >							This would allow the
> > to use av_free() on the AVSubtitles instead of the current
> > avsubtitle_free(), just like it's done for frames.
> 
> Actually, I think the "you must free the structure but not the data it
> points to" API is rather confusing for users, and a "you must free
> everything, there is a convenience function to do it" API would be less
> error-prone.
> 

I was actually wondering for the equivalent for AVFrame, that's how I
realized the difference between the two implementations. It was just a
suggestion for consistency in the API. The confusion can be avoiding by
adding some general design documentation or comments in the examples. I
won't push for this, it really was out of curiosity.

> > Also, adding the AVSubtitle allocation will cause problem for the
> > -fix_sub_duration option recently added in ffmpeg (0cad101e) since we
> > can't swap the AVSubtitle anymore. The AVSubtitle will be allocated only
> > once (just like decoded_frame, we would introduce a decoded_subtitle in
> > the input stream), a pointer copy won't work AFAICT. The question is then,
> > how do we want to support such subtitles copy props? The only similar
> > thing I see for the equivalent AVFrame is the copy props in lavfi. Any
> > suggestion?
> 
> I do not see why it would not be possible to allocate two structures and
> simply swap the pointers to them. Maybe I am missing something?
> 

I think I was confused, a pointer toggle seems to work but maybe I still
misunderstand something.

BTW, any reason this option is not part of avcodec_decode_subtitle2()?

Note: I noticed using -fix_sub_duration in the subviewer FATE test was
skipping one event, but this doesn't look like a regression with my
patches. I didn't look closer though.

-- 
Clément B.
-------------- next part --------------
From 0a0e1992649488980e2cbc4f380a5ad9e892bbe3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux at gmail.com>
Date: Sun, 2 Sep 2012 14:25:28 +0200
Subject: [PATCH 1/3] lavc: add avcodec_alloc_subtitle() and export
 avcodec_get_subtitle_defaults().

---
 doc/APIchanges       |  4 ++++
 libavcodec/avcodec.h | 17 +++++++++++++++++
 libavcodec/utils.c   | 12 +++++++++++-
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/doc/APIchanges b/doc/APIchanges
index f9a624e..a489152 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,10 @@ libavutil:     2011-04-18
 
 API changes, most recent first:
 
+2012-08-13 - xxxxxxx - lavc x.x.100 - avcodec.h
+  Add avcodec_alloc_subtitle()/avcodec_get_subtitle_defaults(), which must be
+  used instead of an AVSubtitle stack allocation.
+
 2012-08-13 - xxxxxxx - lavfi 3.8.100 - avfilter.h
   Add avfilter_get_class() function, and priv_class field to AVFilter
   struct.
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 199dd3b..1c837d3 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -3508,6 +3508,23 @@ AVFrame *avcodec_alloc_frame(void);
  */
 void avcodec_get_frame_defaults(AVFrame *pic);
 
+/**
+ * Allocate an AVSubtitle and set its fields to default values. The resulting
+ * struct can be deallocated by calling avsubtitle_free().
+ *
+ * @return An AVSubtitle filled with default values or NULL on failure.
+ * @see avcodec_get_subtitle_defaults
+ */
+AVSubtitle *avcodec_alloc_subtitle(void);
+
+/**
+ * Set the fields of the given AVSubtitle to default values.
+ *
+ * @param sub The AVSubtitle of which the fields should be set to default
+ *                values.
+ */
+void avcodec_get_subtitle_defaults(AVSubtitle *sub);
+
 #if FF_API_AVCODEC_OPEN
 /**
  * Initialize the AVCodecContext to use the given AVCodec. Prior to using this
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 99e012a..605ef32 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -715,12 +715,22 @@ MAKE_ACCESSORS(AVFrame, frame, int,     decode_error_flags)
 MAKE_ACCESSORS(AVCodecContext, codec, AVRational, pkt_timebase)
 MAKE_ACCESSORS(AVCodecContext, codec, const AVCodecDescriptor *, codec_descriptor)
 
-static void avcodec_get_subtitle_defaults(AVSubtitle *sub)
+void avcodec_get_subtitle_defaults(AVSubtitle *sub)
 {
     memset(sub, 0, sizeof(*sub));
     sub->pts = AV_NOPTS_VALUE;
 }
 
+AVSubtitle *avcodec_alloc_subtitle(void)
+{
+    AVSubtitle *sub = av_malloc(sizeof(*sub));
+
+    if (!sub)
+        return NULL;
+    avcodec_get_subtitle_defaults(sub);
+    return sub;
+}
+
 static int get_bit_rate(AVCodecContext *ctx)
 {
     int bit_rate;
-- 
1.7.12

-------------- next part --------------
From d9129c75ff9e45356b125b8922ee261941af354b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux at gmail.com>
Date: Sun, 2 Sep 2012 14:27:09 +0200
Subject: [PATCH 2/3] ffmpeg: use avcodec_alloc_subtitle() instead of stack
 allocated AVSubtitle.

Also introduce a local decode_subtitle() function for consistency with
audio and video.
---
 ffmpeg.c | 37 ++++++++++++++++++++++++++-----------
 ffmpeg.h |  3 ++-
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/ffmpeg.c b/ffmpeg.c
index 2763db6..e867485 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -1649,12 +1649,27 @@ static int decode_video(InputStream *ist, AVPacket *pkt, int *got_output)
     return ret;
 }
 
+static int decode_subtitle(InputStream *ist, AVPacket *pkt, int *got_output)
+{
+    int ret;
+
+    if (!ist->decoded_subtitle && !(ist->decoded_subtitle = avcodec_alloc_subtitle()))
+        return AVERROR(ENOMEM);
+    avcodec_get_subtitle_defaults(ist->decoded_subtitle);
+
+    update_benchmark(NULL);
+    ret = avcodec_decode_subtitle2(ist->st->codec, ist->decoded_subtitle,
+                                   got_output, pkt);
+    update_benchmark("decode_subtitle %d.%d", ist->file_index, ist->st->index);
+    return ret;
+}
+
 static int transcode_subtitles(InputStream *ist, AVPacket *pkt, int *got_output)
 {
-    AVSubtitle subtitle;
     int64_t pts = pkt->pts;
-    int i, ret = avcodec_decode_subtitle2(ist->st->codec,
-                                          &subtitle, got_output, pkt);
+    int i, ret = decode_subtitle(ist, pkt, got_output);
+    AVSubtitle *subtitle = ist->decoded_subtitle;
+
     if (ret < 0 || !*got_output) {
         if (!pkt->size)
             sub2video_flush(ist);
@@ -1665,25 +1680,25 @@ static int transcode_subtitles(InputStream *ist, AVPacket *pkt, int *got_output)
         if (ist->prev_sub.got_output) {
             int end = av_rescale_q(pts - ist->prev_sub.pts, ist->st->time_base,
                                    (AVRational){ 1, 1000 });
-            if (end < ist->prev_sub.subtitle.end_display_time) {
+            if (end < ist->prev_sub.subtitle->end_display_time) {
                 av_log(ist->st->codec, AV_LOG_DEBUG,
                        "Subtitle duration reduced from %d to %d\n",
-                       ist->prev_sub.subtitle.end_display_time, end);
-                ist->prev_sub.subtitle.end_display_time = end;
+                       ist->prev_sub.subtitle->end_display_time, end);
+                ist->prev_sub.subtitle->end_display_time = end;
             }
         }
         FFSWAP(int64_t,    pts,         ist->prev_sub.pts);
         FFSWAP(int,        *got_output, ist->prev_sub.got_output);
         FFSWAP(int,        ret,         ist->prev_sub.ret);
-        FFSWAP(AVSubtitle, subtitle,    ist->prev_sub.subtitle);
+        FFSWAP(AVSubtitle*,subtitle,    ist->prev_sub.subtitle);
     }
 
-    if (!*got_output || !subtitle.num_rects)
+    if (!*got_output || !subtitle->num_rects)
         return ret;
 
     rate_emu_sleep(ist);
 
-    sub2video_update(ist, &subtitle, pkt->pts);
+    sub2video_update(ist, subtitle, pkt->pts);
 
     for (i = 0; i < nb_output_streams; i++) {
         OutputStream *ost = output_streams[i];
@@ -1691,10 +1706,10 @@ static int transcode_subtitles(InputStream *ist, AVPacket *pkt, int *got_output)
         if (!check_output_constraints(ist, ost) || !ost->encoding_needed)
             continue;
 
-        do_subtitle_out(output_files[ost->file_index]->ctx, ost, ist, &subtitle, pts);
+        do_subtitle_out(output_files[ost->file_index]->ctx, ost, ist, subtitle, pts);
     }
 
-    avsubtitle_free(&subtitle);
+    avsubtitle_free(subtitle);
     return ret;
 }
 
diff --git a/ffmpeg.h b/ffmpeg.h
index cd849c9..dc96886 100644
--- a/ffmpeg.h
+++ b/ffmpeg.h
@@ -203,6 +203,7 @@ typedef struct InputStream {
     int decoding_needed;     /* true if the packets must be decoded in 'raw_fifo' */
     AVCodec *dec;
     AVFrame *decoded_frame;
+    AVSubtitle *decoded_subtitle;
 
     int64_t       start;     /* time when read started */
     /* predicted dts of the next packet read for this stream or (when there are
@@ -235,7 +236,7 @@ typedef struct InputStream {
         int64_t pts;
         int got_output;
         int ret;
-        AVSubtitle subtitle;
+        AVSubtitle *subtitle;
     } prev_sub;
 
     struct sub2video {
-- 
1.7.12

-------------- next part --------------
From e575b99dab98208083e9f17d913a524ff9b4247e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux at gmail.com>
Date: Sun, 2 Sep 2012 14:27:46 +0200
Subject: [PATCH 3/3] ffplay: use avcodec_alloc_subtitle() instead of stack
 allocated AVSubtitle.

---
 ffplay.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/ffplay.c b/ffplay.c
index 2a07be1..75bfb10 100644
--- a/ffplay.c
+++ b/ffplay.c
@@ -115,7 +115,7 @@ typedef struct VideoPicture {
 
 typedef struct SubPicture {
     double pts; /* presentation time stamp for this picture */
-    AVSubtitle sub;
+    AVSubtitle *sub;
 } SubPicture;
 
 typedef struct AudioParams {
@@ -682,7 +682,7 @@ static void blend_subrect(AVPicture *dst, const AVSubtitleRect *rect, int imgw,
 
 static void free_subpicture(SubPicture *sp)
 {
-    avsubtitle_free(&sp->sub);
+    avsubtitle_free(sp->sub);
 }
 
 static void video_image_display(VideoState *is)
@@ -710,7 +710,7 @@ static void video_image_display(VideoState *is)
             if (is->subpq_size > 0) {
                 sp = &is->subpq[is->subpq_rindex];
 
-                if (vp->pts >= sp->pts + ((float) sp->sub.start_display_time / 1000)) {
+                if (vp->pts >= sp->pts + ((float) sp->sub->start_display_time / 1000)) {
                     SDL_LockYUVOverlay (vp->bmp);
 
                     pict.data[0] = vp->bmp->pixels[0];
@@ -721,8 +721,8 @@ static void video_image_display(VideoState *is)
                     pict.linesize[1] = vp->bmp->pitches[2];
                     pict.linesize[2] = vp->bmp->pitches[1];
 
-                    for (i = 0; i < sp->sub.num_rects; i++)
-                        blend_subrect(&pict, sp->sub.rects[i],
+                    for (i = 0; i < sp->sub->num_rects; i++)
+                        blend_subrect(&pict, sp->sub->rects[i],
                                       vp->bmp->w, vp->bmp->h);
 
                     SDL_UnlockYUVOverlay (vp->bmp);
@@ -1248,8 +1248,8 @@ retry:
                         else
                             sp2 = NULL;
 
-                        if ((is->video_current_pts > (sp->pts + ((float) sp->sub.end_display_time / 1000)))
-                                || (sp2 && is->video_current_pts > (sp2->pts + ((float) sp2->sub.start_display_time / 1000))))
+                        if ((is->video_current_pts > (sp->pts + ((float) sp->sub->end_display_time / 1000)))
+                                || (sp2 && is->video_current_pts > (sp2->pts + ((float) sp2->sub->start_display_time / 1000))))
                         {
                             free_subpicture(sp);
 
@@ -1837,21 +1837,24 @@ static int subtitle_thread(void *arg)
         if (pkt->pts != AV_NOPTS_VALUE)
             pts = av_q2d(is->subtitle_st->time_base) * pkt->pts;
 
-        avcodec_decode_subtitle2(is->subtitle_st->codec, &sp->sub,
+        if (!sp->sub && !(sp->sub = avcodec_alloc_subtitle()))
+            return AVERROR(ENOMEM);
+        avcodec_get_subtitle_defaults(sp->sub);
+        avcodec_decode_subtitle2(is->subtitle_st->codec, sp->sub,
                                  &got_subtitle, pkt);
 
-        if (got_subtitle && sp->sub.format == 0) {
+        if (got_subtitle && sp->sub->format == 0) {
             sp->pts = pts;
 
-            for (i = 0; i < sp->sub.num_rects; i++)
+            for (i = 0; i < sp->sub->num_rects; i++)
             {
-                for (j = 0; j < sp->sub.rects[i]->nb_colors; j++)
+                for (j = 0; j < sp->sub->rects[i]->nb_colors; j++)
                 {
-                    RGBA_IN(r, g, b, a, (uint32_t*)sp->sub.rects[i]->pict.data[1] + j);
+                    RGBA_IN(r, g, b, a, (uint32_t*)sp->sub->rects[i]->pict.data[1] + j);
                     y = RGB_TO_Y_CCIR(r, g, b);
                     u = RGB_TO_U_CCIR(r, g, b, 0);
                     v = RGB_TO_V_CCIR(r, g, b, 0);
-                    YUVA_OUT((uint32_t*)sp->sub.rects[i]->pict.data[1] + j, y, u, v, a);
+                    YUVA_OUT((uint32_t*)sp->sub->rects[i]->pict.data[1] + j, y, u, v, a);
                 }
             }
 
-- 
1.7.12

-------------- 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/20120903/36279240/attachment.asc>


More information about the ffmpeg-devel mailing list