[FFmpeg-devel] [PATCH] MOV muxer : Add SoundDescriptionV2 support

[Jai Menon]

On Sat, Nov 28, 2009 at 10:02:17AM -0500, David Conrad wrote:
> On Nov 28, 2009, at 7:07 AM, Jai Menon wrote:
> 
> > On Sat, Nov 28, 2009 at 02:00:39AM -0800, Baptiste Coudurier wrote:
> >> On 11/28/09 1:21 AM, Jai Menon wrote:
> >>> On Sat, Nov 28, 2009 at 12:03:47AM -0800, Baptiste Coudurier wrote:
> >>>> Hi,
> >>>> 
> >>>> On 11/27/09 11:41 PM, Jai Menon wrote:
> >>> 
> >>> [...]
> >>> 
> >>>>> sounddescv2.patch
> >>>>> 
> >>>>> 
> >>>>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> >>>>> index ac6378c..36b156e 100644
> >>>>> --- a/libavformat/movenc.c
> >>>>> +++ b/libavformat/movenc.c
> >>>>> @@ -405,13 +405,21 @@ static int mov_write_glbl_tag(ByteIOContext *pb, MOVTrack *track)
> >>>>> static int mov_write_audio_tag(ByteIOContext *pb, MOVTrack *track)
> >>>>> {
> >>>>>     int64_t pos = url_ftell(pb);
> >>>>> -    int version = track->mode == MODE_MOV&&
> >>>>> +    int version;
> >>>>> +
> >>>>> +    if (track->mode == MODE_MOV&&
> >>>>>         (track->audio_vbr ||
> >>>>>          track->enc->codec_id == CODEC_ID_PCM_S32LE ||
> >>>>> -         track->enc->codec_id == CODEC_ID_PCM_S24LE);
> >>>>> +         track->enc->codec_id == CODEC_ID_PCM_S24LE)) {
> >>>>> +        if (track->timescale>   UINT16_MAX)
> >>>>> +            version = 2;
> >>>> 
> >>>> The stsd v2 must always be used when timescale>  UINT16_MAX
> >>>> regardless of codec.
> >>> 
> >>> Yes, I agree, but right now I just dont have the time required to make
> >>> samples, remux and test against quicktime. Maybe someone who requires
> >>> this for other codecs can add it later?
> >> 
> >> Well, by experience, half-baked solutions for specific codecs rot
> >> forever in the codebase, so I would prefer avoiding it.
> > 
> > I don't think this is that big of a problem. The patch itself would be
> > trivial since it requires setting version outside of the current if
> > block -> ~2 line diff (unless I'm missing something). The issue as I
> > mentioned earlier is that I don't have a QT pro license so I dont
> > have the freedom to generate and test against a multitude of samples.
> > Also, there is the lack of time ;) I'm sure someone who cares about
> > other codecs will want to test it and maybe send a patch. But IMHO
> > that shouldn't get in the way of patches which improve the over all
> > situation.
> > 
> >> 
> >>>>> +        else version = 1;
> >>>>> +    }
> >>>>> 
> >>>>>     put_be32(pb, 0); /* size */
> >>>>> -    put_le32(pb, track->tag); // store it byteswapped
> >>>>> +    if (version == 2)
> >>>>> +        put_le32(pb, AV_RL32("lpcm"));
> >>>> 
> >>>> Technically all codecs can use stsd v2, I have an ALAC sample using
> >>>> stsd v2 since sample rate is 192000. It was just for test :)
> >>> 
> >>> Okay, modified the patch to make it work for ALAC>  96Khz and others.
> >>> 
> >>>>> +    else put_le32(pb, track->tag); // store it byteswapped
> >>>>>     put_be32(pb, 0); /* Reserved */
> >>>>>     put_be16(pb, 0); /* Reserved */
> >>>>>     put_be16(pb, 1); /* Data-reference index, XXX  == 1 */
> >>>>> @@ -444,6 +452,15 @@ static int mov_write_audio_tag(ByteIOContext *pb, MOVTrack *track)
> >>>>>         put_be32(pb, track->sampleSize / track->enc->channels); /* Bytes per packet */
> >>>>>         put_be32(pb, track->sampleSize); /* Bytes per frame */
> >>>>>         put_be32(pb, 2); /* Bytes per sample */
> >>>>> +    } else if (version == 2) {
> >>>>> +        put_be32(pb, 72);
> >>>> 
> >>>> Please split it according to specs:
> >>>> 
> >>>>   SInt16     always3;
> >>>>   SInt16     always16;
> >>>>   SInt16     alwaysMinus2;
> >>>>   SInt16     always0;
> >>>>   UInt32     always65536;
> >>>>   UInt32     sizeOfStructOnly;
> >>> 
> >>> Sorry, I didnt understand this part, 72 is the
> >>> sizeof(SoundDescriptionV2), and the rest are subsequent values.
> >>> 
> >> 
> >> Specs says stsd structure fields must be set to specific values,
> >> which is not done currently.
> > 
> > Ah, yes. Well recent Quicktime versions didnt really care so I didnt
> > bother either. But if it makes you happy, modified patch attached :)
> > 
> > Index: libavformat/movenc.c
> > ===================================================================
> > --- libavformat/movenc.c	(revision 20584)
> > +++ libavformat/movenc.c	(working copy)
> > @@ -405,13 +405,25 @@
> >  static int mov_write_audio_tag(ByteIOContext *pb, MOVTrack *track)
> >  {
> >      int64_t pos = url_ftell(pb);
> > -    int version = track->mode == MODE_MOV &&
> > +    int version;
> 
> version = 0; uninitialized warnings are useful sometimes.

Indeed, very embarassing, BTW, I fixed this on my local branch a while
back but didn't post a new patch because as I said earlier, I'm fine
with showing an error and bailing.
Thanks.

-- 
Jai Menon