[FFmpeg-devel] GSOC update on the CDXL demuxer

Reimar Döffinger Reimar.Doeffinger
Tue Apr 14 20:07:54 CEST 2009


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...



More information about the ffmpeg-devel mailing list