[FFmpeg-devel] [PATCH 1/8] lavf: add internal demuxer helpers for subtitles.
Clément Bœsch
ubitux at gmail.com
Sun Jun 17 19:39:24 CEST 2012
On Sun, Jun 17, 2012 at 02:29:56PM +0200, Nicolas George wrote:
[...]
> > +FFDemuxSubEntry *ff_subtitles_queue_insert_event(FFDemuxSubtitlesQueue *q,
> > + const uint8_t *event, int len,
> > + int merge)
> > +{
> > + FFDemuxSubEntry *subs, *sub;
> > +
> > + if (merge && q->nsub > 0) {
> > + /* merge with previous event */
> > +
> > + uint8_t *tmp;
> > + sub = &q->subs[q->nsub - 1];
> > + tmp = av_realloc(sub->event, sub->len + len + 1);
> > + if (!tmp)
> > + return NULL;
> > + sub->event = tmp;
> > + memcpy(sub->event + sub->len, event, len);
> > + sub->len += len;
> > + } else {
> > + /* new event */
> > +
> > + subs = av_fast_realloc(q->subs, &q->allocated_size,
> > + (q->nsub + 1) * sizeof(*q->subs));
>
> Beware of integer overflows.
>
Added a check.
> > + if (!subs)
> > + return NULL;
> > + q->subs = subs;
> > + sub = &subs[q->nsub++];
> > + memset(sub, 0, sizeof(*sub));
> > + sub->event = av_malloc(len + 1);
> > + sub->len = len;
> > + if (!sub->event)
> > + return NULL;
> > + memcpy(sub->event, event, len);
> > + }
> > + sub->event[sub->len] = 0;
> > + return sub;
> > +}
>
> It looks like the merge and !merge branches are really two completely
> different functions.
>
Yes but this is very handy in some situation, it avoids doing the
branching in the demuxer (see jacosub usage for instance). Maybe we could
have 3 functions, but for now I find it simple enough that way. This is
not public API, it can be changed according the evolution of the needs.
> Also, the "&& q->nsub > 0" looks like defensive programming;
> av_assert0(q->nsub > 0) would look saner to me.
>
Yes I though about that, but this is actually useful sometimes: for
instance if you set your demuxer in an initial merge state until it
reaches a specific point, you don't need to have a first test case.
> > +
> > +static int cmp_pkt_sub(const void *a, const void *b)
> > +{
> > + const int64_t t1 = ((const FFDemuxSubEntry *)a)->start;
> > + const int64_t t2 = ((const FFDemuxSubEntry *)b)->start;
> > + if (t1 == t2)
> > + return 0;
> > + return t1 > t2 ? 1 : -1;
> > +}
>
> The qsort function from the libc is not guaranteed to be stable. In
> subtitles, the order of arrival of simultaneous events may be relevant. A
> secondary compare on the pos field should be enough.
>
Fixed, thanks for pointing this out.
[...]
> > +#ifndef AVFORMAT_SUBTITLES_H
> > +#define AVFORMAT_SUBTITLES_H
> > +
> > +typedef struct {
> > + uint8_t *event; ///< allocated subtitle line, zero terminated
> > + int len; ///< subtitle event length
> > + int64_t pos; ///< offset position
> > + int64_t start; ///< start time (PTS)
> > + int duration; ///< event duration
> > +} FFDemuxSubEntry;
>
> It looks like you did not include some space for side data. As this is an
> internal API, it can be added later though.
>
I didn't have the need for it right now, and since I believe it's
secondary I postponed the addition of such field.
> > +
> > +typedef struct {
> > + FFDemuxSubEntry *subs; ///< array of subtitles events
> > + int nsub; ///< number of events
> > + int allocated_size; ///< allocated size for subs
> > +} FFDemuxSubtitlesQueue;
> > +
> > +/**
> > + * Insert a new subtitle event
> > + *
> > + * @param event the subtitle line, may not be zero terminated
> > + * @param len the length of the event
> > + * @param merge set to 1 if the current event should be concatenated with the
> > + * previous one instead of adding a new entry, 0 otherwise
> > + */
> > +FFDemuxSubEntry *ff_subtitles_queue_insert_event(FFDemuxSubtitlesQueue *q,
> > + const uint8_t *event, int len,
> > + int merge);
>
> Do you insist on such a long name for all the functions? It makes the code
> really hard to indent, especially if one wants to keep the 80-chars limit.
>
Not in particular...
> What about ff_subevents_append? It is not really a queue anyway.
>
I don't like very much the use of "sub" for subtitles, especially when
associated with somehow nonsensical words such as "events".
OTOH, I could drop the "_event" if you want. I'm waiting for a consensus
before I rename all this stuff and update the following commits.
Is the rest of the naming OK? I'd like to avoid too much renames if
possible...
> > +
> > +/**
> > + * Set missing durations and sort subtitles by PTS
> > + */
> > +void ff_subtitles_queue_finalize(FFDemuxSubtitlesQueue *q);
> > +
> > +/**
> > + * Generic read_packet() callback for subtitles demuxers using this queue
> > + * system
> > + */
> > +int ff_subtitles_queue_read_packet(FFDemuxSubtitlesQueue *q,
> > + AVPacket *pkt, int i);
> > +
Note related to your comments in the other patches: the i is moved to the
FFDemuxSubtitlesQueue context, and thus avoid the raised issue.
[...]
--
Clément B.
-------------- next part --------------
From 14d9a7988ff2eea8b2ad2faaae36e537e3fb81dd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20B=C5=93sch?= <ubitux at gmail.com>
Date: Fri, 15 Jun 2012 18:54:52 +0200
Subject: [PATCH 01/10] lavf: add internal demuxer helpers for subtitles.
---
libavformat/Makefile | 1 +
libavformat/subtitles.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++
libavformat/subtitles.h | 70 +++++++++++++++++++++++++++++
3 files changed, 183 insertions(+)
create mode 100644 libavformat/subtitles.c
create mode 100644 libavformat/subtitles.h
diff --git a/libavformat/Makefile b/libavformat/Makefile
index 5e4f002..d53a3e0 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -19,6 +19,7 @@ OBJS = allformats.o \
riff.o \
sdp.o \
seek.o \
+ subtitles.o \
utils.o \
OBJS-$(CONFIG_NETWORK) += network.o
diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c
new file mode 100644
index 0000000..8012039
--- /dev/null
+++ b/libavformat/subtitles.c
@@ -0,0 +1,112 @@
+/*
+ * Copyright (c) 2012 Clément Bœsch
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "avformat.h"
+#include "subtitles.h"
+
+FFDemuxSubEntry *ff_subtitles_queue_insert_event(FFDemuxSubtitlesQueue *q,
+ const uint8_t *event, int len,
+ int merge)
+{
+ FFDemuxSubEntry *subs, *sub;
+
+ if (merge && q->nsub > 0) {
+ /* merge with previous event */
+
+ uint8_t *tmp;
+ sub = &q->subs[q->nsub - 1];
+ tmp = av_realloc(sub->event, sub->len + len + 1);
+ if (!tmp)
+ return NULL;
+ sub->event = tmp;
+ memcpy(sub->event + sub->len, event, len);
+ sub->len += len;
+ } else {
+ /* new event */
+
+ if (q->nsub >= INT_MAX/sizeof(*q->subs) - 1)
+ return NULL;
+ subs = av_fast_realloc(q->subs, &q->allocated_size,
+ (q->nsub + 1) * sizeof(*q->subs));
+ if (!subs)
+ return NULL;
+ q->subs = subs;
+ sub = &subs[q->nsub++];
+ memset(sub, 0, sizeof(*sub));
+ sub->event = av_malloc(len + 1);
+ sub->len = len;
+ if (!sub->event)
+ return NULL;
+ memcpy(sub->event, event, len);
+ }
+ sub->event[sub->len] = 0;
+ return sub;
+}
+
+static int cmp_pkt_sub(const void *a, const void *b)
+{
+ const FFDemuxSubEntry *s1 = a;
+ const FFDemuxSubEntry *s2 = b;
+ if (s1->start == s2->start) {
+ if (s1->pos == s2->pos)
+ return 0;
+ return s1->pos > s2->pos ? 1 : -1;
+ }
+ return s1->start > s2->start ? 1 : -1;
+}
+
+void ff_subtitles_queue_finalize(FFDemuxSubtitlesQueue *q)
+{
+ int i;
+
+ qsort(q->subs, q->nsub, sizeof(*q->subs), cmp_pkt_sub);
+ for (i = 0; i < q->nsub; i++)
+ if (q->subs[i].duration == -1 && i < q->nsub - 1)
+ q->subs[i].duration = q->subs[i + 1].start - q->subs[i].start;
+}
+
+int ff_subtitles_queue_read_packet(FFDemuxSubtitlesQueue *q, AVPacket *pkt)
+{
+ int res;
+ FFDemuxSubEntry *sub = q->subs + q->current_sub;
+
+ if (q->current_sub == q->nsub)
+ return AVERROR_EOF;
+ res = av_new_packet(pkt, sub->len + 1);
+ if (res)
+ return res;
+ memcpy(pkt->data, sub->event, sub->len + 1);
+ pkt->flags |= AV_PKT_FLAG_KEY;
+ pkt->pos = sub->pos;
+ pkt->pts = pkt->dts = sub->start;
+ pkt->duration = sub->duration;
+ q->current_sub++;
+ return 0;
+}
+
+void ff_subtitles_queue_free(FFDemuxSubtitlesQueue *q)
+{
+ int i;
+
+ for (i = 0; i < q->nsub; i++)
+ av_freep(&q->subs[i].event);
+ av_freep(&q->subs);
+ q->nsub = q->allocated_size = q->current_sub = 0;
+}
diff --git a/libavformat/subtitles.h b/libavformat/subtitles.h
new file mode 100644
index 0000000..dfe0d6c
--- /dev/null
+++ b/libavformat/subtitles.h
@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2012 Clément Bœsch
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#ifndef AVFORMAT_SUBTITLES_H
+#define AVFORMAT_SUBTITLES_H
+
+#include <stdint.h>
+#include "avformat.h"
+
+typedef struct {
+ uint8_t *event; ///< allocated subtitle line, zero terminated
+ int len; ///< subtitle event length
+ int64_t pos; ///< offset position
+ int64_t start; ///< start time (PTS)
+ int duration; ///< event duration
+} FFDemuxSubEntry;
+
+typedef struct {
+ FFDemuxSubEntry *subs; ///< array of subtitles events
+ int nsub; ///< number of events
+ int allocated_size; ///< allocated size for subs
+ int current_sub; ///< current position for the read packet callback
+} FFDemuxSubtitlesQueue;
+
+/**
+ * Insert a new subtitle event
+ *
+ * @param event the subtitle line, may not be zero terminated
+ * @param len the length of the event
+ * @param merge set to 1 if the current event should be concatenated with the
+ * previous one instead of adding a new entry, 0 otherwise
+ */
+FFDemuxSubEntry *ff_subtitles_queue_insert_event(FFDemuxSubtitlesQueue *q,
+ const uint8_t *event, int len,
+ int merge);
+
+/**
+ * Set missing durations and sort subtitles by PTS, and then byte position
+ */
+void ff_subtitles_queue_finalize(FFDemuxSubtitlesQueue *q);
+
+/**
+ * Generic read_packet() callback for subtitles demuxers using this queue
+ * system
+ */
+int ff_subtitles_queue_read_packet(FFDemuxSubtitlesQueue *q, AVPacket *pkt);
+
+/**
+ * Remove and free all the events
+ */
+void ff_subtitles_queue_free(FFDemuxSubtitlesQueue *q);
+
+#endif /* AVFORMAT_SUBTITLES_H */
--
1.7.10.4
-------------- 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/20120617/beb80d09/attachment.asc>
More information about the ffmpeg-devel
mailing list