[FFmpeg-devel] [PATCH] wmapro decoder
Sascha Sommer
saschasommer
Fri Aug 21 19:33:45 CEST 2009
Hi,
I attached an updated patch. As you might have already noticed, I do not have
much time to work on this project so please keep the focus on the important
things. I do not mind if Diego or someone else fixes the alignment, coding
style, typo and wording problems directly in the SVN sources or if these
things are pointed out in a single review but it is very frustrating to
resubmit this patch again and again and to synchonize the main and soc svn
for things that in the end do not give any real benefit. This is an
unacceptable waste of my time. Thanks.
> > > > + /** decode transform type */
> > > > + if (chgroup->num_channels == 2) {
> > > > + if (get_bits1(&s->gb)) {
> > > > + if (get_bits1(&s->gb)) {
> > > > + av_log_ask_for_sample(s->avctx,
> > > > + "unsupported channel transform
> > > > type\n"); + }
> > > > + } else {
> > > >
> > > > + if (s->num_channels == 2) {
> > > > + chgroup->transform = 1;
> > > > + } else {
> > > > + chgroup->transform = 2;
> > > > + /** cos(pi/4) */
> > > > + chgroup->decorrelation_matrix[0] =
> > > > 0.70703125; + chgroup->decorrelation_matrix[1]
> > > > = -0.70703125; +
> > > > chgroup->decorrelation_matrix[2] = 0.70703125; +
> > > > chgroup->decorrelation_matrix[3] = 0.70703125; +
> > > > }
> > >
> > > why the special handling of 2 vs. >2 channels here?
> >
> > When the stream has only 2 channels, the channels are M/S stereo coded
> > (transform 1)
> > When the stream has more than 2 channels, the matrix multiplication is
> > used. for the 2 channels that contain data for the current subframe
> > length/offset. (num_channels in the channel group != num_channels in the
> > stream)
>
> iam not sure if we talk about the same thing or if i misunderstand you but
> the 2channel in a subframe and the M/S case look pretty much the same to me
> if so i wonder why they are not handled by the same code ...
>
I'm not sure I really did the change you meant. See the attached patch.
> > + uint16_t channel_len; ///< channel frame
> > length in samples
>
> do we need that in the context or can it be a local var?
> also if i understand the code the variable name is not too good
>
Changed to be a local var.
> > + uint16_t decoded_samples; ///< already
> > processed samples
>
> the _number_of_ already processed samples ?
>
Fixed.
> > + int8_t transmit_sf; ///< transmit
> > scale factors for the current subframe
>
> flag indicating that ... ?
>
Fixed.
> > + uint8_t bits_per_sample;
>
> i think this should be explained more completely as similarly named vars
> exist in AVCodecContext, that is in how far is that different ...
>
Fixed.
> > + int8_t num_channels;
>
> same issue
>
Fixed.
> > + uint8_t max_num_subframes; ///< maximum number
> > of subframes
>
> that doxy is redundant
>
> > + int8_t num_possible_block_sizes; ///< number of
> > distinct block sizes that can be found in the file
> >
> > + uint16_t min_samples_per_subframe; ///< minimum samples
> > per subframe
>
> same
>
Removed.
> > + int16_t
> > sf_offsets[WMAPRO_BLOCK_SIZES][WMAPRO_BLOCK_SIZES][MAX_BANDS]; ///< scale
> > factor resample matrix
>
> does this really need to be 16bit ?
>
Changed.
> > +#ifdef DEBUG
> > + /** dump the extradata */
> > + for (i=0 ; i<avctx->extradata_size ; i++)
> > + av_log(avctx, AV_LOG_DEBUG, "[%x] ",avctx->extradata[i]);
> > + av_log(avctx, AV_LOG_DEBUG, "\n");
> > +#endif
>
> dprintf()
>
Changed.
> > + /** subframe info */
> > + log2_num_subframes = ((s->decode_flags & 0x38) >> 3);
>
> log2_max_num_subframes ?
>
Changed.
> > + while (missing_samples > 0) {
>
> isnt that the same as a simple check on min_channel_len, which at the end
> should be frame len?
>
It is but I don't think the code will become cleaner when min_channel_len is
checked.
> >
> > + if (channels_for_cur_subframe == 1 ||
> > + min_samples == missing_samples)
>
> these 2 look redundant
> also the condition for reading the mask could just be used instead of
> the temporary var read_channel_mask
Did the second thing. Please explain why these 2 look redundant.
> > + /* 1 bit indicates if the subframe length is zero */
>
> no, its never zero, that would also make no sense
>
Oups. Fixed.
> > + /** add subframes to the individual channels */
> > + if (min_channel_len == chan->channel_len) {
> > + --channels_for_cur_subframe;
> > + if (channel_mask & (1<<channels_for_cur_subframe)) {
>
> id do a get_bits1() here instead of loading it in a mask and then
> extracting it
> (btw you can just do GetBitContext mask_gb= *s->gb)
Then this would reintroduce the check for the case that the subframe is used
for all channels.
> > +
> > + if (vlctable) {
> > + run = coef1_run;
> > + level = coef1_level;
> > + } else {
> > + run = coef0_run;
> > + level = coef0_level;
> > + }
>
> have you tried run = coef_run[vlctable] ... or so?
> i mean it might be faster as it doesnt do a conditional branch ...
>
That does not seem to change much.
> > + if (i >= s->num_bands) {
> > + av_log(s->avctx,AV_LOG_ERROR,
> > + "invalid scale factor coding\n");
> > + return AVERROR_INVALIDDATA;
> > + } else
> > + s->channel[c].scale_factors[i] += (val ^ sign) -
> > sign;
>
> the else is superflous
>
Fixed.
> > + s->channel[c].coeffs =
> > &s->channel[c].out[(s->samples_per_frame>>1) +
> > + offset];
> >
> > + memset(s->channel[c].coeffs,0,sizeof(float) * subframe_len);
>
> cant that be avoided?
>
Not directly. One would have to compensate this in three other places (before
rl_decode, in vector decode when value == 0 and when the coeffs are not
transmitted.
Regards
Sascha
-------------- next part --------------
A non-text attachment was scrubbed...
Name: wmapro_try5.patch
Type: text/x-diff
Size: 61452 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090821/ec88423c/attachment.patch>
More information about the ffmpeg-devel
mailing list