[FFmpeg-cvslog] r15536 - trunk/libavformat/mov.c

[Baptiste Coudurier]

Michael Niedermayer wrote:
> On Fri, Oct 03, 2008 at 06:34:12PM -0700, Baptiste Coudurier wrote:
>> Michael Niedermayer wrote:
>>> On Fri, Oct 03, 2008 at 09:37:42PM +0200, bcoudurier wrote:
>>>> Author: bcoudurier
>>>> Date: Fri Oct  3 21:37:41 2008
>>>> New Revision: 15536
>>>>
>>>> Log:
>>>> set sample rate for amr to allow stream copy since no decoder is present,
>>>> 3gp does not store sample rate in stsd in fact. Fixes #674.
>>>>
>>>>
>>>> Modified:
>>>>    trunk/libavformat/mov.c
>>>>
>>>> Modified: trunk/libavformat/mov.c
>>>> ==============================================================================
>>>> --- trunk/libavformat/mov.c	(original)
>>>> +++ trunk/libavformat/mov.c	Fri Oct  3 21:37:41 2008
>>>> @@ -991,6 +991,9 @@ static int mov_read_stsd(MOVContext *c, 
>>>>      case CODEC_ID_AMR_WB:
>>>>          st->codec->frame_size= sc->samples_per_frame;
>>>>          st->codec->channels= 1; /* really needed */
>>>> +        /* force sample rate for amr, stsd in 3gp does not store sample rate */
>>>> +        if (st->codec->codec_id != CODEC_ID_QCELP)
>>>> +            st->codec->sample_rate = 8000;
>>>>          break;
>>> I was wondering if not maybe something like
>>> if(!st->codec->sample_rate)
>>>     st->codec->sample_rate = 8000;
>> This would only work if sample_rate is not set previously when reading
>> stsd, I agree setting it is not correct in some cases, however, I fear
>> indentifing 3gp in all cases will be complicated even with brands, and
>> in most cases timescale/sample_rate is correctly set to sample rate like
>> suggested in specs (but not mandatory).
> 
> IIRC the iso based file formats have default values for things,
> thus things should IIRC either be set to te default or to the correct
> value.
> If iam correct then if(sample_rate != default) -> sample_rate is correct
> and should not be overridden by 8khz

I totally agree, now in 3gp in fact, this field is not sample_rate, it
is timescale and must have the same value than in header, that was
overlooked.
In 3g2, for amrwb+ (which we do not support), it _is_ sample_rate.

While in iso media:
"In audio tracks, the sampling rate of the audio should be used as the
timescale of the media, and also documented in the samplerate field here."

So usually this field is set right also in 3gp.

> Of course i do not know if existing files follow the spec, also ive not
> checked the spec iam just writing from memory ...
> 
> 
>>> or
>>> if(3gp)
>>>     st->codec->sample_rate = 8000;
>>> would be cleaner ?
>> Humm aac in 3gp may not be 8000 :/
> 
> aac also wouldnt run the AMR/QCELP part of the switch() :)

Ah ok.

> but still i agree that identifying 3gp isnt easy :(

Yes :/

>> In the end, I was thinking that the code was this way since a long time,
>> and definitely amrnb and amrwb only support mono 8000 hz, so I decided
>> to go for this solution.
> 
> amrwb was 16khz before not 8khz ...

Right, this is a bug, fixed, and I understand better now your comment,
my intent was to fix sample to the _right_ value, not default.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Smartjog USA Inc.                                http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA