[FFmpeg-devel] [PATCH 02/11] lavf: add internal demuxer helpers for subtitles.

Clément Bœsch ubitux at gmail.com
Sat Jun 23 00:56:57 CEST 2012


On Sat, Jun 23, 2012 at 12:34:59AM +0200, Nicolas George wrote:
[...]
> > +AVPacket *ff_subtitles_queue_insert(FFDemuxSubtitlesQueue *q,
> > +                                    const uint8_t *event, int len, int merge)
> > +{
> > +    AVPacket *subs, *sub;
> > +
> > +    if (merge && q->nsub > 0) {
> > +        /* merge with previous event */
> > +
> > +        int old_len;
> > +        sub = &q->subs[q->nsub - 1];
> > +        old_len = sub->size;
> > +        if (av_grow_packet(sub, len) < 0)
> > +            return NULL;
> > +        memcpy(sub->data + old_len, event, 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++];
> > +        if (av_new_packet(sub, len) < 0)
> > +            return NULL;
> > +        sub->destruct = NULL;
> > +        sub->flags |= AV_PKT_FLAG_KEY;
> > +        sub->pts = sub->dts = 0;
> > +        memcpy(sub->data, event, len);
> > +    }
> > +    return sub;
> > +}
> 
> I am still not convinced that the merge feature belongs here. AFAICS, the
> only place you use it (in the reimplementation of jacosub), you could just
> replace it with an immediate new call to read_line. It would be more
> efficient to avoid repeated reallocs.
> 

I'm using that feature in SAMI and RT as well. I'm not sure to understand
what you propose.

> > +
> > +static int cmp_pkt_sub(const void *a, const void *b)
> > +{
> > +    const AVPacket *s1 = a;
> > +    const AVPacket *s2 = b;
> > +    if (s1->pts == s2->pts) {
> > +        if (s1->pos == s2->pos)
> > +            return 0;
> > +        return s1->pos > s2->pos ? 1 : -1;
> > +    }
> > +    return s1->pts > s2->pts ? 1 : -1;
> > +}
> 
> Could be written:
> 
> return s1->pts < s2->pts ? -1 : s1->pts > s2->pts ? +1 :
>        s1->pos < s2->pos ? -1 : s1->pos > s2->pos ? +1 : 0;
> 
> 

I prefer the current form, which I find more obvious.

> > +
> > +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].pts - q->subs[i].pts;
> > +}
> 
> Could also reallocate allocated_size down to nsub.
> 

I thought the memory was not an issue in this case... :-)

> > +
> > +int ff_subtitles_queue_read_packet(FFDemuxSubtitlesQueue *q, AVPacket *pkt)
> > +{
> > +    AVPacket *sub = q->subs + q->current_sub;
> > +
> > +    if (q->current_sub == q->nsub)
> > +        return AVERROR_EOF;
> > +    *pkt = *sub;
> > +    pkt->dts = pkt->pts;
> > +    q->current_sub++;
> > +    return 0;
> > +}
> > +
> > +void ff_subtitles_queue_free(FFDemuxSubtitlesQueue *q)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < q->nsub; i++)
> > +        av_destruct_packet(&q->subs[i]);
> > +    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..3373027
> > --- /dev/null
> > +++ b/libavformat/subtitles.h
> > @@ -0,0 +1,61 @@
> > +/*
> > + * 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 {
> > +    AVPacket *subs;         ///< array of subtitles packets
> > +    int nsub;               ///< number of subtitles packets
> > +    int allocated_size;     ///< allocated size for subs
> > +    int current_sub;        ///< current position for the read packet callback
> > +} FFDemuxSubtitlesQueue;
> 
> Nit: I really think that "queue" is misleading.
> 

Well I like the semantic given by "queue" here: we construct a chain of
subtitles which will be raise node by node. The implementation is also not
a List, and I don't like what Array suggests (direct access to a node X),
so I'll keep Queue.

> > +
> > +/**
> > + * 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
> > + */
> > +AVPacket *ff_subtitles_queue_insert(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);
> 
> Nit: in oo-speak, finalize means that the object is about to be destroyed
> and can not be used again. Maybe _settle? Not very important.
> 

"Finalize" makes more sense to me…

Sorry I don't want to end up in a never ending bikeshed thread for such
thing, so I'm just against all these nits.

[...]

-- 
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/20120623/80938f24/attachment.asc>


More information about the ffmpeg-devel mailing list