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

[Michael Niedermayer]

On Thu, Jul 30, 2015 at 11:17:28AM -0400, Ganesh Ajjanagadde wrote:
> 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?

usig AVRational is difficult as AVCodecContext is a public struct,
changing the type of a field in it would break ABI&API

I dont really have a great idea on how to solve this issue but maybe
the bitrate could be used directly instead of using
bits_per_coded_sample as an intermediate
or one could write a G729 specific check, that would work but
it become more ugly
the data_size per sample_count has to be within the range of
bitrates supported by the codec, its way outside for this file so
should be detectable that way
the check could also be limited to the actually set bitrate if that
is trusted instead of the bitrates that a codec supports

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150730/d9773a85/attachment.sig>