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

[Michael Niedermayer]

On Sun, Jun 17, 2012 at 12:25:20AM +0200, Clément Bœsch wrote:
> On Sat, Jun 16, 2012 at 07:29:11PM +0200, Michael Niedermayer wrote:
> > On Fri, Jun 15, 2012 at 07:29:08PM +0200, Clément Bœsch wrote:
[...]
> > 
> > > +
> > > +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 ?
> 
> Yeah, I moved the qsort() before; this could be made configurable at some
> point depending on the format.
> 
> > also it might make sense to check if things are ordered before
> > doing a qsort in case the array is scaned fully
> > 
> 
> Since I moved the qsort() before I can't do that anymore. I thought about
> adding a flag in the insert function when notifying a different PTS, but
> that function doesn't take a PTS (since it can only be known later
> sometimes). qsort() should be fast enough if the array is ordered
> anyway... Also, it's only at init time.

ok


> 
> > 
> > > +}
> > > +
> > > +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
> 
> Doing this will require a bigger allocated queue (AVPacket is bigger), and
> I think it's important to see which fields can be accessed and modified.
> The heavy part in creating the AVPacket is the data allocation, and it
> will be needed anyway because a same sub event can be raised several times
> (with a seek).

fine with me

about the patch no more comments from me, it looks fine

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- 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/20120617/c929390c/attachment.asc>