[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