[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