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

Wolfram Gloger wmglo at dent.med.uni-muenchen.de
Mon Jun 18 15:24:33 CEST 2012


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.

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

What is the unit of 'start', please?

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).

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

> ...

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

This is all just MHO, I don't intend a bikeshed-type discussion.

Regards,
Wolfram.


More information about the ffmpeg-devel mailing list