[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