[Ffmpeg-devel] [PATCH] FLV common header.

[Michael Niedermayer]

Hi

On Fri, Dec 01, 2006 at 08:04:54PM -0800, Allan Hsu wrote:
> The attached patch adds a common header, flv.h, that defines various  
> common FLV container values between the FLV muxer and demuxer. It  
> moves some defines from the muxer to enums in the common header and  
> it changes the hardcoded values in both flvenc.c and flvdec.c to the  
> new values in the common header. It should not change the behavior of  
> any of the code.
> 
> If this patch is acceptable, my intention is to use the common header  
> values in a forthcoming series of onMetaData tag reading patches.

patch ok except a few nitpicks


[...]
> @@ -158,14 +159,14 @@
>          st->codec->time_base= (AVRational){1,1000};
>      }
>  //    av_log(NULL, AV_LOG_DEBUG, "%d %X %d \n", is_audio, flags, st->discard);
> -    if(  (st->discard >= AVDISCARD_NONKEY && !((flags >> 4)==1 ||  is_audio))
> -       ||(st->discard >= AVDISCARD_BIDIR  &&  ((flags >> 4)==3 && !is_audio))
> +    if(  (st->discard >= AVDISCARD_NONKEY && !((flags & FLV_VIDEO_FRAMETYPE_MASK) == FLV_VIDEO_FRAME_KEY ||  is_audio))
> +       ||(st->discard >= AVDISCARD_BIDIR  &&  ((flags & FLV_VIDEO_FRAMETYPE_MASK) == FLV_VIDEO_FRAME_DISPOSABLE_INTER && !is_audio))

is_audio was nicely aligned, now it looks a little less nice, also the line is somewhat long ...


[...]
> @@ -173,31 +174,31 @@
>      if(is_audio){
>          if(st->codec->sample_rate == 0){
>              st->codec->codec_type = CODEC_TYPE_AUDIO;
> -            st->codec->channels = (flags&1)+1;
> -            if((flags >> 4) == 5)
> +            st->codec->channels = (flags & FLV_AUDIO_CHANNELCOUNT_MASK) == FLV_AUDIO_CHANNELS_STEREO ? 2 : 1;
> +            if((flags & FLV_AUDIO_CODECID_MASK) == FLV_AUDIO_CODECID_NELLYMOSER_8HZ_MONO)

this too gets a little long
FLV_        (hmm dunno)
AUDIO_      (well stereo video is possible but hmm)
CHANNELS_   (stereo/mono implicates channels so this is redundant)
STEREO      (this part is needed)

so FLV_STEREO seems like a better choice as its less chars to read and still
contains all the information

FLV_AUDIO_CHANNELCOUNT_MASK
maybe CHANNELCOUNT_MASK or FLV_CHANNELCOUNT_MASK would be better


>                  st->codec->sample_rate= 8000;
>              else
> -                st->codec->sample_rate = (44100<<((flags>>2)&3))>>3;
> -            switch(flags >> 4){/* 0: uncompressed 1: ADPCM 2: mp3 5: Nellymoser 8kHz mono 6: Nellymoser*/
> -            case 0: if (flags&2) st->codec->codec_id = CODEC_ID_PCM_S16BE;
> +                st->codec->sample_rate = (44100 << ((flags & FLV_AUDIO_SAMPLERATE_MASK) >> FLV_AUDIO_SAMPLERATE_OFFSET) >> 3);
> +            switch(flags & FLV_AUDIO_CODECID_MASK) {
> +            case FLV_AUDIO_CODECID_UNCOMPRESSED: if (flags & FLV_AUDIO_SAMPLESIZE_MASK) st->codec->codec_id = CODEC_ID_PCM_S16BE;
>                      else st->codec->codec_id = CODEC_ID_PCM_S8; break;
> -            case 1: st->codec->codec_id = CODEC_ID_ADPCM_SWF; break;
> -            case 2: st->codec->codec_id = CODEC_ID_MP3; st->need_parsing = 1; break;
> +            case FLV_AUDIO_CODECID_ADPCM: st->codec->codec_id = CODEC_ID_ADPCM_SWF; break;
> +            case FLV_AUDIO_CODECID_MP3: st->codec->codec_id = CODEC_ID_MP3; st->need_parsing = 1; break;
>              // this is not listed at FLV but at SWF, strange...
> -            case 3: if (flags&2) st->codec->codec_id = CODEC_ID_PCM_S16LE;
> +            case FLV_AUDIO_CODECID_UNCOMPRESSED_LE: if (flags & FLV_AUDIO_SAMPLESIZE_MASK) st->codec->codec_id = CODEC_ID_PCM_S16LE;

FLV_AUDIO_CODECID_UNCOMPRESSED_LE -> FLV_CODECID_PCM_LE
FLV_AUDIO_CODECID_MP3 ->FLV_CODECID_MP3
...

you can also choose other names if you like, its just
that the ones you propose are a little overlong which makes the code
somewhet hard to read IMHO

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is