[FFmpeg-devel] GSOC update on the CDXL demuxer

Erion Omeri erion.omeri
Tue Apr 14 21:47:26 CEST 2009


Wow, that is really nice and thorough feedback. Thanks for the comments
everyone!

Erion




On Tue, Apr 14, 2009 at 1:07 PM, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de>wrote:

> On Tue, Apr 14, 2009 at 11:39:59AM -0500, Erion Omeri wrote:
> > /**
> >  * @file libavformat/cdxl.c
> >  * CDXL file demuxer
> >  * by Erion Omeri (erion.omeri at gmail.com)
> >  *
> >  * Record Header
> >  * ---------------------------------------
> >  * byte 0        File type
> >  * byte 1        Info byte
> >  *   bits 0-2      Video encoding
> >  *   bit 3         Stereo flag
> >  *   bits 5-7      Plane arrangement
> >  * bytes 2-5     Current chunk size
> >  * bytes 6-9     Previous chunk size
> >  * bytes 10-11   Reserved
> >  * bytes 12-13   Current frame number (1 for first frame)
> >  * bytes 14-15   Video width
> >  * bytes 16-17   Video height
> >  * bytes 18-19   Number of bit planes
> >  * bytes 20-21   Palette size in bytes
> >  * bytes 22-23   Sound size in bytes
> >  * bytes 24-31   Reserved
> >  */
>
> I'd suggest just referencing the MultimediaWiki page here.
>
> > #define CDXL_FRAME_RATE 11025
>
> FRAME_RATE is a really bad name for audio, "sample rate" is what it is
> called.
>
> > #define CDXL_PALETE_SIZE 0x20
>
> It's spelled PALETTE (with double T)
>
> > #undef printf
>
> Just use av_log, e.g. the ugly way av_log(NULL, AV_LOG_ERROR, ...) as a
> first step.
> Also here and in many other places you have trailing whitespace, i.e.
> spaces after the last visible character in the line, a patch containing
> such would be rejected by the version control's precommit verification.
>
> > typedef struct{
> >     char Type;          // byte 0
> >     char Info;          // byte 1 ( bit 0,1,2, bit 3, bit 5,6,7)
>
> char can be signed or unsigned, depending on compiler. Most of the time,
> uint8_t or int8_t is more appropriate.
> Also in this case I see no reason why this would have to be exactly
> eight bits, and thus you should better be using int.
>
> >     long CurrSize;      // byte 2, 3, 4, 5
> >     long PrevSize;      // byte 6, 7, 8, 9
>
> long can be 32 or 64 bit, it almost never is what you want.
>
> >     long Reserved1;     // byte 24, 25, 26, 27,
> >     long Reserved2;     // byte 28, 29, 30, 31
>
> If you don't use them, don't read them and don't store them.
> Also FFmpeg in general does not use CamelCase, i.e.
> CurrSize should be curr_size instead.
>
> >     #if 0
> >     debug_header(cdxl);
> >     #endif
>
> The # of preprocessor directives must be the first character in the
> line. If you want to indent them you'd do it like this:
> > #   if 0
> But usually I wouldn't do it at all.
> Also a less ugly way to achieve this often is doing it like this:
> if (0) debug_header(cdxl);
>
> >     /** If Info=00001000 than stereo **/
> >     st->codec->channels = CDXL_MONO;
> >     if( CDXL_STEREO_MASK == ( CDXL_STEREO_MASK & cdxl->Info ) )
> >         st->codec->channels = CDXL_STEREO;
>
> Well, 1 channel is mono, two channels is stereo IMO those CDXL_MONO and
> CDXL_STEREO only make it harder to understand.
> Also
> > if( CDXL_STEREO_MASK == ( CDXL_STEREO_MASK & cdxl->Info ) )
> is the same as
> > if( 8 == ( 8 & cdxl->Info ) )
> (which btw. means "MASK" is a bad name, that is usually assumed to mean
> it is more than one bit, CDXL_STEREO_FLAG should be a better name).
> This again is the same as
> > if( 8 & cdxl->Info != 0 )
> which is the same as
> > if (cdxl->Info & CDXL_STEREO_MASK)
> which I think sure is easier to read.
>
> >    /** Skip Palette **/
> >    url_fskip(pb, CDXL_PALETE_SIZE);
> >
> >    cdxl->VideoFrameCount += 1;
> >
> >    /*Skip Video */
> >    videoSize = cdxl->CurrSize - cdxl->RawSoundSize - CDXL_PALETE_SIZE -
> CDXL_HEADER_SIZE;
> >    url_fskip(pb, videoSize);
>
> Splitting apart video and palette is likely to do more bad than good,
> just do
> > videoSize = cdxl->CurrSize - cdxl->RawSoundSize - CDXL_HEADER_SIZE;
> > url_fskip(pb, videoSize);
> and have them both handled in one piece. That will be the same when you
> add actual video support, you will want the palette and the data in one
> packet.
>
> >     ret = av_get_packet(s->pb, pkt, cdxl->RawSoundSize);
> >     if(ret != cdxl->RawSoundSize)
> >         return AVERROR(EIO);
>
> Bad idea.
> First, you are leaking memory if ret > 0 but < cdxl->RawSoundSize.
> Second, you are losing information about the specific error that caused
> av_get_packet to fail (e.g. it might have run out of memory and not an
> IO error).
> Lastly, at least for now IMHO libavformat demuxers are supposed to
> return partial frame data.
> So what you should be using is
> > if (ret < 0)
> >     return ret;
>
> >     /** If stereao, the actually sound size is half **/
> >     if( 2 == s->streams[CDXL_AUDIO_STREAM_ID]->codec->channels  )
> >         cdxl->AudioFrameCount += ( cdxl->RawSoundSize / 2 );
> >     else
> >         cdxl->AudioFrameCount += cdxl->RawSoundSize;
>
> A simpler way to write it would be
>
> > cdxl->AudioFrameCount += cdxl->RawSoundSize /
> s->streams[CDXL_AUDIO_STREAM_ID]->codec->channels
>
> it's still quite ugly though, I'd suggest storing the channel count
> somewhere in CDXLContext to make it less ugly.
>
> >     //cdxl_read_only_header(pb, cdxl);
>
> Well, you will have to read at least "chunk size" and "sound size"
> values from the header and skip the rest, otherwise you'll be reading
> your audio data from the wrong place...
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>



More information about the ffmpeg-devel mailing list