[FFmpeg-devel] [PATCH] Demuxer for Leitch/Harris' VR native stream format (LXF)

[Tomas Härdin]

I've been unable to answer mail for a while, so I'll answer both Diego
and Michael below to avoid the thread splitting in two:

On Tue, 2010-08-31 at 14:58 +0200, Diego Biurrun wrote: 
> On Fri, Aug 27, 2010 at 05:00:39PM +0200, Tomas H?rdin wrote:
> > 
> > --- a/libavformat/avformat.h
> > +++ b/libavformat/avformat.h
> > @@ -22,7 +22,7 @@
> >  #define AVFORMAT_AVFORMAT_H
> >  
> >  #define LIBAVFORMAT_VERSION_MAJOR 52
> > -#define LIBAVFORMAT_VERSION_MINOR 78
> > +#define LIBAVFORMAT_VERSION_MINOR 79
> >  #define LIBAVFORMAT_VERSION_MICRO  3
> 
> Reset the micro version if you bump minor.

Done.

> > --- /dev/null
> > +++ b/libavformat/lxfdec.c
> > @@ -0,0 +1,335 @@
> > +/*
> > + * LXF demuxer.
> 
> Drop this pointless period.

Done.

> > +//returns number of bits set in value
> > +static int num_set_bits(uint32_t value) {
> 
> { on the next line

Done.

> > +    for(ret = 0; value; ret += (value & 1), value >>= 1);
> 
> for (
> 
> more below 

Searched and replaced.

> > +//reads and checksums packet header. returns format and size of payload
> > +static int get_packet_header(AVFormatContext *s, unsigned char *header, uint32_t *format)
> 
> long line

Fixed.

> > +    LXFDemuxContext *lxf = s->priv_data;
> > +    ByteIOContext *pb = s->pb;
> 
> vertically align the =

Done.

> 
> > +    switch(AV_RL32(&header[16])) {
> 
> switch (

Searched and replaced.

> > +        if (lxf->bps != 16 && lxf->bps != 20 && lxf->bps != 24 && lxf->bps != 32) {
> > +            av_log(s, AV_LOG_WARNING, "only 16-, 20-, 24- and 32-bit PCM currently supported\n");
> 
> Please break long lines where easily possible, same in other places.

Done in various places.

> > +        st->codec->codec_type = AVMEDIA_TYPE_AUDIO;
> > +        st->codec->sample_rate = LXF_SAMPLERATE;
> > +        st->codec->channels = lxf->channels;
> 
> vertically align the =
> 
> Diego

Aligned in a few more places as well.

On Thu, 2010-09-02 at 17:13 +0200, Michael Niedermayer wrote:
> On Fri, Aug 27, 2010 at 05:00:39PM +0200, Tomas H?rdin wrote:
> > [...]
> > Some limitations and quirks:
> > 
> > * VBI data is not handled, but skipped. I'm not sure how it should be
> > implemented. I also have no samples with such data
> 
> does any of our demuxers support returning VBI data?

Well, some do output AVMEDIA_TYPE_DATA. However, searching for "VBI"
reveals that the only cases where VBI data is mentioned is in muxers
where the active height of the video has to be set. In other words,
making sure the player hides the VBI lines.

Note that I have samples with visible VBI lines, but they don't have the
data available in that data field.

> [...]
> > * Audio is stored in a planar fashion and is always 48 kHz. The audio
> > data is de-planerized by the demuxer
> 
> this needs a FIXME comment so we can remove it once we support planar
> audio or you can implement planar audio if you like ...

Added a FIXME. Planar audio is not very high on my list of priorities at
the moment, but maybe it should be taken into account in that audio
AVFrame thread?

> > * 16-, 20-, 24- and 32-bit PCM is supported. I doubt there are any files
> > in the wild with any other sample depths. 20-bit PCM is in-place
> > expanded to 24-bit by the demuxer (the top 4 MSBs are copied to the
> > bottom 4 LSBs)
> 
> this should probably be handeld similar to CODEC_ID_PCM_DVD

Good idea. I looked at that codec, and unfortunately it packs its
samples slightly differently. This means a new PCM codec would be
required, like CODEC_ID_PCM_LXF.

I'll hold off on a new codec though until I get some feedback on the
attached patch.

> > * It is possible for a packet to not contain data for all channels (it
> > uses a bitmask). In that case I'm not sure how to behave - probably
> > output silence for those channels
> 
> do you have a sample that uses this feature?

Nope.

My theory is that they wanted support for connecting/disconnecting AES
cables on the fly. I lack access to the hardware at the moment though.
Maybe later I can try it and see what happens.

> > [...]
> > diff --git a/doc/general.texi b/doc/general.texi
> > index b775e68..522dccd 100644
> > --- a/doc/general.texi
> > +++ b/doc/general.texi
> > @@ -114,6 +114,8 @@ library:
> >      @tab A format used by libvpx
> >  @item LMLM4                     @tab   @tab X
> >      @tab Used by Linux Media Labs MPEG-4 PCI boards
> > + at item LXF                       @tab X @tab
> > +    @tab VR native stream format, used by Leitch/Harris' video servers.

I noticed this ticks "Encoder", not "Decoder". Fixed.

> > [...]
> > +static int lxf_probe(AVProbeData *p)
> > +{
> > +    if (p->buf_size < LXF_IDENT_LENGTH)
> > +        return 0;
> 
> unneeded

Right - PROBE_BUF_MIN is quite a bit larger. Fixed.

> > +
> > +    if (!memcmp(p->buf, LXF_IDENT, LXF_IDENT_LENGTH))
> > +        return AVPROBE_SCORE_MAX;
> > +
> > +    return 0;
> > +}
> > +
> 
> > +//returns zero if checksum is OK, non-zero otherwise
> 
> doxy

Why? It's not exported. Or did you mean the comment style in general? I
doxyfied check_checksum() and num_set_bits() as an example.

> > +static int check_checksum(unsigned char *header)
> > +{
> > +    int x, sum = 0;
> > +
> > +    for (x = 0; x < LXF_PACKET_HEADER_SIZE; x += 4)
> > +        sum += AV_RL32(&header[x]);
> > +
> > +    return sum;
> 
> unsigned sum, signed numbers have undefined overflow in C IIRC and
> if one is picky

Fixed.

> > +}
> > +
> 
> > +//returns number of bits set in value
> > +static int num_set_bits(uint32_t value) {
> > +    int ret;
> > +
> > +    for(ret = 0; value; ret += (value & 1), value >>= 1);
> > +
> > +    return ret;
> > +}
> 
> if we dont have a population count function yet, than one should be added
> to some header in libavutil

I couldn't find one. That probably belongs in its own thread though.

Which files would such a function belong in - intmath.h/c, common.h or
somewhere else? Also, which name would be best: ff_count_bits(),
av_count_bits() or something else?

Anyway, see attached patch. Ran tests for good measure.

/Tomas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: lxfdec2.patch
Type: text/x-patch
Size: 15064 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100907/17457643/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100907/17457643/attachment.pgp>