[Ffmpeg-devel] [PATCH] non conformance to 3GP format (3GP TS 26.244 v 6.5.0 2006/06)

[Michael Niedermayer]

Hi

On Wed, Jul 26, 2006 at 06:02:58PM +0200, ffmpeg wrote:
> Hello,
> 
> I would like to submit for review (and maybe integration) the following 
> patch:
> 
> 
> Purpose:  Fix some non conformance to 3GP format  (3GP TS 26.244 v 6.5.0 
> 2006/06)
> Applies:   libavcodec/movenc.c {SVN 5720}, (test within trunk SVN 5726)
> Testing:   
>      - Regression test OK;
>      - 3GP file check with some commercial 3gp analysis tools
>      - play on QuickTime
>      - play on HP OCMP Video 1.0
> Authors:   Francois Draperi
> Discussion:
>    Some fields (like in 'samr' atom) are not set to 0; "3GP TS 26.244" 
> enforces those values to be null.     
>    The patch simply put 0 in the needed audio and video atom fields, for
>    3GP files (and only them), and for AMR_NB mode (and only this codec).
>    Maybe someone may also ask to restrict those changes to the '-strict 
> very' mode, but is it useful ?
> 
> Hope that help
> Francois Draperi.
> 

[...]
> +              (track->enc->codec_id == CODEC_ID_AMR_NB && track->mode != MODE_3GP) ; 

trailing whitespace


> +   
>      int version = track->mode == MODE_MOV &&
>          (vbr ||
>           track->enc->codec_id == CODEC_ID_PCM_S32LE ||
>           track->enc->codec_id == CODEC_ID_PCM_S24LE);
>  
> +    
> +

cosmetical changes


[...]
>  
> +    printf("#### vbr is %d\n\n\n", vbr);

printf is forbidden, this actually should not have compiled at all


>      if(vbr) {
>          put_be16(pb, 0xfffe); /* compression ID (vbr)*/
>      } else {
> @@ -608,16 +613,25 @@
>      put_be16(pb, 0); /* Reserved */
>      put_be16(pb, 1); /* Data-reference index */
>  
> -    put_be16(pb, 0); /* Codec stream version */
> -    put_be16(pb, 0); /* Codec stream revision (=0) */
> -    put_tag(pb, "FFMP"); /* Vendor */
> -    if(track->enc->codec_id == CODEC_ID_RAWVIDEO) {
> +    /* FIX BEGIN as defined in  3GP TS 26.244 v 6.5.0 2006/06 ,for s263 atom, following  must be set to 0  [Francois Dr.]  */
> +    if ( track->mode == MODE_3GP) {    
> +      put_be32(pb, 0);

indention in ffmpeg should be consitant, if a file uses 4-spaces, changes
must use 4 spaces too unless you want to avoid unneeded whitespaces changes
of existing code, but you didnt do that here ...


> +      put_be32(pb, 0);
> +      put_be32(pb, 0);
> +      put_be32(pb, 0);
> +    } else {
> +      put_be16(pb, 0); /* Codec stream version */
> +      put_be16(pb, 0); /* Codec stream revision (=0) */

the above 2 are correct for all cases so this is code duplication


> +      put_tag(pb, "FFMP"); /* Vendor */
> +      if(track->enc->codec_id == CODEC_ID_RAWVIDEO) {
>          put_be32(pb, 0); /* Temporal Quality */
>          put_be32(pb, 0x400); /* Spatial Quality = lossless*/
> -    } else {
> -        put_be32(pb, 0x200); /* Temporal Quality = normal */
> +      } else {
> +	put_be32(pb, 0x200); /* Temporal Quality = normal */

tabs are forbidden

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