[FFmpeg-devel] [PATCH] Demuxer for Leitch/Harris' VR native stream format (LXF)
Michael Niedermayer
michaelni
Fri Sep 10 13:43:35 CEST 2010
On Tue, Sep 07, 2010 at 04:17:10PM +0200, Tomas H?rdin wrote:
> 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.
>
[...]
> > > * 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.
we are trying to have all comments in doxy format where its applicable
and it is for documenting a function even if internal
>
> > > +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?
av_popcount()
would be similar to gccs __builtin_popcount()
[...]
> +//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 ?
> +
> + break;
> + case 1:
> + //audio
> + if (!(st = s->streams[1])) {
> + av_log(s, AV_LOG_INFO, "got audio packet, but no audio stream present\n");
> + break;
> + }
> +
> + //set codec based on specified audio bitdepth
> + //we only support tightly packed 16-, 20-, 24- and 32-bit PCM at the moment
> + *format = AV_RL32(&header[40]);
> + lxf->bps = (*format >> 6) & 0x3F;
> +
> + if (lxf->bps != (*format & 0x3F)) {
> + av_log(s, AV_LOG_WARNING, "only tightly packed PCM currently supported\n");
> + return AVERROR_PATCHWELCOME;
> + }
> +
> + 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");
> + return AVERROR_PATCHWELCOME;
> + }
> +
> + //round bps up to next multiple of eight
> + if (lxf->bps <= 16) {
> + st->codec->codec_id = CODEC_ID_PCM_S16LE;
> + st->codec->bits_per_coded_sample = 16;
> + } else if (lxf->bps <= 24) {
> + st->codec->codec_id = CODEC_ID_PCM_S24LE;
> + st->codec->bits_per_coded_sample = 24;
> + } else {
> + st->codec->codec_id = CODEC_ID_PCM_S32LE;
> + st->codec->bits_per_coded_sample = 32;
> + }
> +
> + track_size = AV_RL32(&header[48]);
> + samples = track_size * 8 / lxf->bps;
> +
> + //use audio packet size to determine video standard
> + //for NTSC we have one 8008-sample audio frame per five video frames
> + if (samples == LXF_SAMPLERATE * 5005 / 30000) {
> + av_set_pts_info(s->streams[0], 64, 1001, 30000);
> + s->duration = av_rescale_q(lxf->num_frames,
> + (AVRational){1001,30000}, AV_TIME_BASE_Q);
> + } else {
> + //assume PAL, but warn if we don't have 1920 samples
> + if (samples != LXF_SAMPLERATE / 25)
> + av_log(s, AV_LOG_WARNING,
> + "video doesn't seem to be PAL or NTSC. guessing PAL\n");
> +
> + av_set_pts_info(s->streams[0], 64, 1, 25);
> + s->duration = av_rescale_q(lxf->num_frames,
> + (AVRational){1,25}, AV_TIME_BASE_Q);
> + }
you should set the duration of the AVStream and let the s->duration for the
core to fill (should be simpler)
[...]
> +static int lxf_read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> + LXFDemuxContext *lxf = s->priv_data;
> + ByteIOContext *pb = s->pb;
> + unsigned char header[LXF_PACKET_HEADER_SIZE];
> + int ret, stream, ofs = 0, format;
> + AVStream *ast = NULL;
> +
> + if ((ret = get_packet_header(s, header, &format)) < 0)
> + return ret;
> +
> + av_log(s, AV_LOG_DEBUG, "got %d B packet\n", ret);
> +
> + if ((stream = AV_RL32(&header[16])) == 1 && !(ast = s->streams[stream])) {
> + av_log(s, AV_LOG_ERROR, "got audio packet without having an audio stream\n");
> + return -1;
> + }
> +
> + if (ast) {
> + if (lxf->bps == 20) {
> + //20-bit PCM gets inplace-expanded to 24-bit
> + //make room for 20% more data in the packet than we read
> + ofs = ret / 5;
> + }
> +
> + //make sure the data fits in the buffer
> + if (ofs + ret > LXF_MAX_AUDIO_PACKET) {
> + av_log(s, AV_LOG_ERROR, "audio packet too large (%i > %i)\n",
> + ofs + ret, LXF_MAX_AUDIO_PACKET);
> + return -1;
ofs + ret can overflow
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100910/3f960cef/attachment.pgp>
More information about the ffmpeg-devel
mailing list