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

Michael Niedermayer michaelni at gmx.at
Sat Jun 16 19:29:11 CEST 2012


On Fri, Jun 15, 2012 at 07:29:08PM +0200, Clément Bœsch 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_realloc_f(q->subs, q->nsub + 1, sizeof(*q->subs));

av_fast_realloc() should be faster


> +        if (!subs)
> +            return NULL;
> +        q->subs = subs;
> +        sub = &subs[q->nsub++];
> +        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)
> +{
> +    return ((const FFDemuxSubEntry*)a)->start - ((const FFDemuxSubEntry*)b)->start;
> +}

could be writen like:
static int cmp_pkt_sub(const FFDemuxSubEntry *a, const FFDemuxSubEntry *b)
{
    return a->start - b->start;
}

but would then need a cast of some sort when used to avoid a warning


> +
> +void ff_subtitles_queue_finalize(FFDemuxSubtitlesQueue *q)
> +{
> +    int i;
> +
> +    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;
> +    qsort(q->subs, q->nsub, sizeof(*q->subs), cmp_pkt_sub);

shouldnt the duration fill in be after qsort ?
also it might make sense to check if things are ordered before
doing a qsort in case the array is scaned fully


> +}
> +
> +int ff_subtitles_queue_read_packet(FFDemuxSubtitlesQueue *q,
> +                                   AVPacket *pkt, int i)
> +{
> +    int res;
> +    FFDemuxSubEntry *sub = q->subs + i;
> +
> +    if (i == q->nsub)
> +        return AVERROR_EOF;
> +    res = av_new_packet(pkt, sub->len + 1);
> +    if (res)
> +        return res;
> +    memcpy(pkt->data, sub->event, sub->len);
> +    pkt->data[sub->len] = 0;
> +    pkt->flags |= AV_PKT_FLAG_KEY;
> +    pkt->pos = sub->pos;
> +    pkt->pts = pkt->dts = sub->start;
> +    pkt->duration = sub->duration;

this would be simpler if each FFDemuxSubEntry contained a AVPacket
and used its fields where applicable
or maybe plain AVPackets would be sufficient and FFDemuxSubEntry
wouldnt be needed but that may be too unflexible dunno


[...]
> +/**
> + * Insert a new subtitle event
> + */
> +FFDemuxSubEntry *ff_subtitles_queue_insert_event(FFDemuxSubtitlesQueue *q,
> +                                                 const uint8_t *event, int len,
> +                                                 int merge);

the parameters should be documented, from just reading this i wouldnt
know what "merge" does

[...]

otherwise LGTM if noone else has comments

thanks

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120616/52f7b520/attachment.asc>


More information about the ffmpeg-devel mailing list