[Ffmpeg-devel] Re: [PATCH] non conformance to 3GP format (3GP TS 26.244 v 6.5.0 2006/06) 2nd try
Michael Niedermayer
michaelni
Sun Aug 6 20:41:32 CEST 2006
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
More information about the ffmpeg-devel
mailing list