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

Clément Bœsch ubitux at gmail.com
Mon Jun 18 22:53:00 CEST 2012


On Mon, Jun 18, 2012 at 03:24:33PM +0200, Wolfram Gloger wrote:
> Sorry for the delay.
> 
> Clément Bœsch <ubitux at gmail.com> writes:
> 
> > +typedef struct {
> > +    uint8_t *event;     ///< allocated subtitle line, may not be zero-terminated
> > +    int len;            ///< subtitle event length
> > +    int64_t pos;        ///< offset position
> > +    int64_t start;      ///< timestamp start
> > +    int duration;       ///< event duration
> > +} FFDemuxSubEntry;
> 
> Generally, I'd personally prefer just to use AVPacket here.  Like you
> said yes it's bigger, but just for the really low bandwidth subtitles I
> do not think it is worth the effort.
> 
> But ok, if we want a new struct (and I sympathise with "see what is
> accessed" aka a more object-oriented approach) this really begs for more
> generic names IMHO.
> 

That's not the only reasons: event will always be a null terminated string
(yes I fixed the doxy-comment), and len is the length, not the size (it
doesn't count the zero, and is used to avoid some calls to strlen).

> I'd replace 'event' with 'data' and 'len' with 'size'.
> 

It's not really the same thing as said above.

> What is the unit of 'start', please?
> 

Should be in AV_TIME_BASE, unless I'm missing something.

> Could we define that unit to be arbitrary and only be used for sorting
> 'comparable' FFDemuxSubEntry s (with pos as tie breaker)?  If yes, that
> would enable us to derive AVPacket from FFDemuxSubEntry and AVPacket.pts
> would be 'FFDemuxSubEntry.start' (sure, that would a huge change, but
> might eventually be worth it).
> 

The only benefit I see in using AVPacket is that it will save 4 lines of
diff, but OTOH it will double the required allocated size of subtitles and
make not obvious how to use the AVPacket fields.

> Now about the name 'FFDemuxSubEntry', I think I would vastly
> prefer 'FFDemuxEvent' or 'FFDemuxEntry' or 'FFDemuxData'.
> 

Whatever, I don't mind much renaming stuff, maybe I'll choose
FFDemuxSubtitle if that's find with everyone.

Please don't forget this really is private API.

> > ...
> 
> None of your functions look subtitle-specific to me..
> 

But they are, in the sense that it only works with strings and they're
meant to be a full list of "text events" ordered by time (and pos).

Maybe you could use it to make some kind of event log system in a
decentralized server application, but in the case of FFmpeg, it looks
fairly appropriate to use this API only for text subtitles.

[...]

-- 
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/20120618/8da2ec70/attachment.asc>


More information about the ffmpeg-devel mailing list