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

[David Conrad]

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.