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

[Tomas Härdin]

On Mon, 2010-09-13 at 22:06 +0200, Michael Niedermayer wrote:
> On Mon, Sep 13, 2010 at 08:44:00PM +0100, M?ns Rullg?rd wrote:
> > Michael Niedermayer <michaelni at gmx.at> writes:
> > 
> > > On Mon, Sep 13, 2010 at 10:56:43AM +0100, M?ns Rullg?rd wrote:
> > >> Tomas H?rdin <tomas.hardin at codemill.se> writes:
> > > [...]
> > >> >> > +//reads and checksums packet header. returns format and size of payload
> > >> >> > +static int get_packet_header(AVFormatContext *s, unsigned char *header,
> > >> >> > +                             uint32_t *format)
> > >> >> > +{
> > >> >> > +    LXFDemuxContext *lxf = s->priv_data;
> > >> >> > +    ByteIOContext   *pb  = s->pb;
> > >> >> > +    int size, track_size, samples;
> > >> >> > +    AVStream *st;
> > >> >> > +
> > >> >> > +    if (get_buffer(pb, header, LXF_PACKET_HEADER_SIZE) != LXF_PACKET_HEADER_SIZE)
> > >> >> > +        return AVERROR(EIO);
> > >> >> > +
> > >> >> > +    if (memcmp(header, LXF_IDENT, LXF_IDENT_LENGTH)) {
> > >> >> > +        av_log(s, AV_LOG_ERROR, "packet ident mismatch - out of sync?\n");
> > >> >> > +        return -1;
> > >> >> > +    }
> > >> >> > +
> > >> >> > +    if (check_checksum(header))
> > >> >> > +        av_log(s, AV_LOG_ERROR, "checksum error\n");
> > >> >> > +
> > >> >> > +    *format = AV_RL32(&header[32]);
> > >> >> > +    size    = AV_RL32(&header[36]);
> > >> >> > +
> > >> >> > +    //type
> > >> >> > +    switch (AV_RL32(&header[16])) {
> > >> >> > +    case 0:
> > >> >> > +        //video
> > >> >> > +        //skip VBI data and metadata
> > >> >> > +        url_fskip(pb, AV_RL32(&header[44]) + AV_RL32(&header[52]));
> > >> >> 
> > >> >> cant this lead to a backward seek and thus infinite loop ?
> > >> >
> > >> > Yes, assuming AV_RL32() returns signed. Fixed by casting to uint32_t and
> > >> > splitting up into two skips.
> > >
> > > split is ugly
> > >
> > >> 
> > >> AV_RL32() returns an unsigned value.
> > >
> > > maybe it should but i dont think it does
> > >
> > > #   define AV_RL32(x)                           \
> > >     ((((const uint8_t*)(x))[3] << 24) |         \
> > >      (((const uint8_t*)(x))[2] << 16) |         \
> > >      (((const uint8_t*)(x))[1] <<  8) |         \
> > >       ((const uint8_t*)(x))[0])
> > 
> > You are right.  Should we change it?
> 
> i do not know, but at least we should make sure all implementations of AV_R*
> are consistent in terms of signedness

I went with a double cast to eliminate the split seek and guarantee that
they are added together correctly.

I decided to give the CODEC_ID_PCM_LXF idea a try. Two patches attached:
one adds a new PCM decoder called "pcm_lxf", which performs both
unpacking and de-planerization. The other patch is a demuxer that makes
use of this decoder, which made the code a bit simpler.

I also cleaned up the code a bit more, especially regarding handling the
four supported bit depths. It also makes use of av_popcount() now.

/Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lxfdec5.patch
Type: text/x-patch
Size: 13761 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100915/b943d2dd/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pcm_lxf.patch
Type: text/x-patch
Size: 3953 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100915/b943d2dd/attachment-0001.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/20100915/b943d2dd/attachment.pgp>