[FFmpeg-devel] [PATCH] adpcm-ima-wav fix bytespersec field in wav header

[Timofei V. Bondarenko]

Michael Niedermayer wrote:
> On Wed, Nov 14, 2007 at 03:30:32PM +0300, Timofei V. Bondarenko wrote:
>> Michael Niedermayer wrote:
>>> On Tue, Nov 13, 2007 at 05:00:25PM +0300, Timofei V. Bondarenko wrote:
>>>> Hi,
>>>>
>>>> The patch does write correct bitrate/bytespersecond field in the WAV 
>>>> header for adpcm-ima-wav codec.
>>>>
>>>> Regards,
>>>> 	Tim.
>>>>
>>>> PS
>>>> I've already sent this patch one week ago but didn't received any 
>>>> comments.
>>>> Index: libavformat/riff.c
>>>> ===================================================================
>>>> --- libavformat/riff.c	(revision 10939)
>>>> +++ libavformat/riff.c	(working copy)
>>>> @@ -280,6 +280,8 @@
>>>>          enc->codec_id == CODEC_ID_PCM_S32LE ||
>>>>          enc->codec_id == CODEC_ID_PCM_S16LE) {
>>>>          bytespersec = enc->sample_rate * blkalign;
>>>> +    } else if (enc->codec_id == CODEC_ID_ADPCM_IMA_WAV) {
>>>> +        bytespersec = enc->sample_rate * enc->block_align / 
>>>> enc->frame_size;
>>>>      } else {
>>>>          bytespersec = enc->bit_rate / 8;
>>>>      }
>>> does the following work as well?
>>>  if (enc->codec_id == CODEC_ID_PCM_U8 ||
>>>         enc->codec_id == CODEC_ID_PCM_S24LE ||
>>>         enc->codec_id == CODEC_ID_PCM_S32LE ||
>>>         enc->codec_id == CODEC_ID_PCM_S16LE ||
>>> +       enc->enc->block_align
>> enc->block_align?
> 
> yes
> 
> 
>>>         ) {
>>>         bytespersec = enc->sample_rate * blkalign;
>>>     } else {
>> No, it doesn't.
>> the block_align is a seek unit which corresponds to 1 frame for simple PCM 
>> formats, and enc->frame_size frames (about =1017) for adpcm.
>>
>>> if so, i think it would be preferable as its more general, your solution
>>> would need a check per codec
>> I'd prefer a more general solution too because the next thing i want to 
>> post is a similar fix to avcodec_string().
>>
>> See the next version:
>> the pcm_encode_init() set the both frame_size and block_align,
>> so whole PCM family should be handled right.
>>
>> I'm not sure about other codecs the new formula becames default for many 
>> codecs instead of enc->bit_rate/8.
>> Is (block_align != 0) check safe enough?
>>
>> At least for WMA this change affects regression.
> 
> if the patch affects regressions then the regression checksums should be
> updated in the patch as well
> 
> [...]

The question is about more general solution.
I cannot verify whether the 'generalized' version correct for all 
affected formats.

So would not be easier to keep the per-codec check?
If this check acceptable then my original patch contains all regression 
updates.

If not then may be you know a better check to choose enc->bit_rate/8 vs.
enc->sample_rate * enc->block_align / enc->frame_size ?


Regards,
	Tim.