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

[Michael Niedermayer]

Hi

On Sun, Aug 06, 2006 at 04:13:07PM +0200, Baptiste Coudurier wrote:
> Hi
> 
> Michael Niedermayer wrote:
> > Hi
> > 
> > On Fri, Jul 28, 2006 at 03:13:52PM +0200, ffmpeg wrote:
> >> Hello,
> >>
> >> I would like to submit for review (and maybe integration) the following 
> >> patch,
> >> This is my 2nd try, now based on 5829 (trying to follow last version)
> >>
> >> 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 5829)
> >> 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 ?
> >> ToDo:
> >>    As suggested by Baptiste C., should also applies to mp4v, mp4a, and 
> >> potentially avc1. I do not know enough those, so I let someone else do 
> >> the change.
> > 
> > patch looks ok, assuming regression tests pass, and whoever applies it 
> > should also fix the indention in a second, seperate commit
> > 
> > [...]
> 
> Ok, here is an updated patch, which should make files conforming to all
> standards, 3GP, MP4.
> 
> There are some FIXME's, does anyone have more informations that I could
> not find ? Especially concerning audio > 2 channels, and compressor name
> in mp4.
> 
> Michael, do you think we should keep using AVI fourcc ? The more I read
> specs and the more I feel we should not do that and strictly mux
> according to specs.

hmm, where do we use avi fourcc in mov/mp4 except if theres no mov fourcc
maybe?


[...]
> @@ -360,16 +360,14 @@
>      put_be16(pb, 0); /* Revision level */
>      put_be32(pb, 0); /* Reserved */
>  
> -    put_be16(pb, track->enc->channels); /* Number of channels */
> -    /* TODO: Currently hard-coded to 16-bit, there doesn't seem
> -                 to be a good way to get number of bits of audio */
> -    put_be16(pb, 0x10); /* Reserved */
> -
> -    if(vbr) {
> -        put_be16(pb, 0xfffe); /* compression ID (vbr)*/
> -    } else {
> -        put_be16(pb, 0); /* compression ID (= 0) */
> -    }
> +    /* FIXME not sure, ISO 14496-12 specifies either 1 or 2, no samples found where channels > 2 */
> +    if (track->mode == MODE_MOV || (track->mode == MODE_MP4 && track->enc->channels < 2))
> +        put_be16(pb, track->enc->channels);
> +    else
> +        put_be16(pb, 2); /* Reserved */

does the spec just not specify how to store streams with more then 2 channels
or does it explicitly say that >2 channels audio should have 2 in the channels
field?


> +    /* FIXME 8 bit for 'raw ' */
> +    put_be16(pb, 16);
> +    put_be16(pb, vbr ? 0xfffe : 0); /* Compression ID */

this is a cosmetic change, ive no objections, your new code is simpler
and nicer but cosmetics should be seperate from functional changes


>      put_be16(pb, 0); /* packet size (= 0) */
>      put_be16(pb, track->timescale); /* Time scale */
>      put_be16(pb, 0); /* Reserved */
> @@ -610,13 +608,19 @@
>  
>      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) {
> -        put_be32(pb, 0); /* Temporal Quality */
> -        put_be32(pb, 0x400); /* Spatial Quality = lossless*/
> +    if (track->mode == MODE_MOV) {
> +        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 */
> +            put_be32(pb, 0x200); /* Spatial Quality = normal */
> +        }
>      } else {
> -        put_be32(pb, 0x200); /* Temporal Quality = normal */
> -        put_be32(pb, 0x200); /* Spatial Quality = normal */
> +        put_be32(pb, 0); /* Reserved */
> +        put_be32(pb, 0); /* Reserved */
> +        put_be32(pb, 0); /* Reserved */
>      }

ive no objections to this but IMO this should be split in 2 commits
1. adding the  if (track->mode == MODE_MOV) { ... } else { ... }
2. reindenting the code within the new if()
it would be much more readable on cvslog that way

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