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

[Michael Niedermayer]

On Mon, Nov 19, 2007 at 04:55:44PM +0300, Timofei V. Bondarenko wrote:
> 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.

can you verify that the current code in svn is correct for all affected
formats? no?
then what are you arguing here about? theres A and B you cant verify either


> 
> So would not be easier to keep the per-codec check?

well for that you first would have to verify that NO clean and general
solution exists
after all clean solutions are prefered over complicated and messy ones
so they require strong proofs to be accepted

PS: wma in wav / wma in asf should be tested (with WMP) to ensure it works
the rest should be fine

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

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071119/4ca26550/attachment.pgp>