[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