[FFmpeg-devel] [PATCH 2/2] riffdec: override bits per sample for G.729 as well

[Ganesh Ajjanagadde]

On Thu, Jul 30, 2015 at 9:31 AM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Wed, Jul 29, 2015 at 12:28:47AM -0400, Ganesh Ajjanagadde wrote:
>> May be used to fix Ticket4577
>>
>> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> ---
>>  libavformat/riffdec.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/riffdec.c b/libavformat/riffdec.c
>> index 7eecdb2..43d4cfc 100644
>> --- a/libavformat/riffdec.c
>> +++ b/libavformat/riffdec.c
>> @@ -176,8 +176,8 @@ int ff_get_wav_header(AVFormatContext *s, AVIOContext *pb,
>>          codec->channels    = 0;
>>          codec->sample_rate = 0;
>>      }
>> -    /* override bits_per_coded_sample for G.726 */
>> -    if (codec->codec_id == AV_CODEC_ID_ADPCM_G726 && codec->sample_rate)
>> +    /* override bits_per_coded_sample for G.726  and G.729 */
>> +    if ((codec->codec_id == AV_CODEC_ID_ADPCM_G726 || codec->codec_id == AV_CODEC_ID_G729) && codec->sample_rate)
>>          codec->bits_per_coded_sample = codec->bit_rate / codec->sample_rate;
>
> This feels somewhat wrong
> G729 doesnt really code each sample in 1 or 0.8 bits, also the
> 6.4kbit/sec mode would end with a 0 here

Ok. I wonder what is the best way to move forward with this.
Recall the intent of the patch series: the first patch (which you
merged and added a correction)
uses this bits_per_coded_sample as an estimate for the "avg" bits per
coded sample
as a heuristic for detecting wrong sample counts.
The trouble is that unless bit_per_coded_sample is set to the avg bit rate,
we are not doing a best effort correction to incorrect number of samples.
And if we set it above that value, this field really has no meaning.
So in my limited understanding, I would have thought
bits_per_coded_sample represents the "avg" bits per sample,
and not necessarily coding each sample in 1 or 0.8 bits.
In any case, your example shows that unless this field is made an AVRational,
this field could be meaningless for certain codecs.
IMHO, currently setting it to 16 for G.729 is far more incorrect than
lending it the "avg" meaning.
Any suggestions/thoughts on making this field an AVRational?

>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Those who are too smart to engage in politics are punished by being
> governed by those who are dumber. -- Plato
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>