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; > > > /**. > > > //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 > >