[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