[FFmpeg-devel] [PATCH] wmapro decoder
Sascha Sommer
saschasommer
Sat Aug 29 23:51:54 CEST 2009
Hi,
new patch attached...
> > > > + 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.
>
> i think the whole tile code is messy and could be simplified, iam of course
> not saying that above change would be a good idea but something should
> change to make things simpler ...
Hopefully it is better now.
> > initialization) */ + uint8_t lossless;
> > ///< lossless mode
>
> never set or did i miss it?
>
Removed. The lossless variable never did much.
> > + int8_t num_possible_block_sizes; ///< number of
> > distinct block sizes that can be found in the file
>
> i think the doxy is poor because it could also apply to 4 = [1,2,16,32]
>
Variable removed from the context.
> > + /** calculate subframe len bits */
> > + if (s->lossless) {
> > + subframe_len_bits = av_log2(s->max_num_subframes - 1) + 1;
> > + } else {
> > + if (s->max_num_subframes == 16)
> > + subframe_len_zero_bit = 1;
> > + subframe_len_bits = av_log2(av_log2(s->max_num_subframes)) +
> > 1; + }
>
> this maybe can be moved to decode_init()
>
Done.
> > +
> > + /** loop until the frame data is split between the subframes */
> > + while (missing_samples > 0) {
> > + unsigned int channel_mask = 0;
> > + int min_channel_len;
> > + int channels_for_cur_subframe = 0;
> > + int subframe_len;
> > + /** minimum number of samples that need to be read */
> > + int min_samples = s->min_samples_per_subframe;
> > +
> >
> > + if (fixed_channel_layout) {
> > + channels_for_cur_subframe = s->num_channels;
> > + min_channel_len = num_samples[0];
> > + } else {
> > + min_channel_len = s->samples_per_frame;
> > + /** find channels with the smallest overall length */
> > + for (c = 0; c < s->num_channels; c++) {
> > + if (num_samples[c] <= min_channel_len) {
> > + if (num_samples[c] < min_channel_len) {
> > + channels_for_cur_subframe = 0;
> > + min_channel_len = num_samples[c];
> > + }
> > + ++channels_for_cur_subframe;
> > + }
> > + }
> > + }
>
> is the fixed_channel_layout special case needed?
> also cant the initial min_channel_len be INT_MAX ?
I rewrote most parts of decode_tilehdr. In this case INT_MAX could have been
used. The special case was needed.
>
> > + min_samples *= channels_for_cur_subframe;
> > +
> > + /** For every channel with the minimum length, 1 bit
> > + might be transmitted that informs us if the channel
> > + contains a subframe with the next subframe_len. */
> > + if (fixed_channel_layout || channels_for_cur_subframe == 1
> > || + min_samples ==
> > missing_samples) { + channel_mask = -1;
> > + } else {
> > + channel_mask = get_bits(&s->gb,
> > channels_for_cur_subframe); + if (!channel_mask) {
> > + av_log(s->avctx, AV_LOG_ERROR,
> > + "broken frame: zero frames for subframe_len\n");
> > + return AVERROR_INVALIDDATA;
> > + }
> > + }
> > +
> > + /** if we have the choice get next subframe length from the
> > + bitstream */
> > + if (min_samples != missing_samples) {
> >
> > + int log2_subframe_len = 0;
> > + /* 1 bit indicates if the subframe is of maximum length
> > */ + if (subframe_len_zero_bit) {
> > + if (get_bits1(&s->gb)) {
> > + log2_subframe_len = 1 +
> > + get_bits(&s->gb, subframe_len_bits-1);
> > + }
> > + } else
> > + log2_subframe_len = get_bits(&s->gb,
> > subframe_len_bits);
> >
> > +
> > + if (s->lossless) {
> > + subframe_len =
> > + s->samples_per_frame / s->max_num_subframes;
> > + subframe_len *= log2_subframe_len + 1;
> > + } else {
> > + subframe_len =
> > + s->samples_per_frame / (1 << log2_subframe_len);
> > + }
>
> one of these multiplies by log2_subframe_len the other divides, is that
> intended?
Lossless support removed. I don't remember if this worked correctly or not.
> > +/**
> > + *@brief Extract scale factors from the bitstream.
> > + *@param s codec context
> > + *@return 0 on success, < 0 in case of bitstream errors
> > + */
> > +static int decode_scale_factors(WMA3DecodeContext* s)
> > +{
> > + int i;
> > +
> > + /** should never consume more than 5344 bits
> > + * MAX_CHANNELS * (1 + MAX_BANDS * 23)
> > + */
> > +
> > + for (i = 0; i < s->channels_for_cur_subframe; i++) {
> > + int c = s->channel_indexes_for_cur_subframe[i];
> > + int* sf;
> > + int* sf_end = s->channel[c].scale_factors + s->num_bands;
> > +
> > + /** resample scale factors for the new block size */
> > + if (s->channel[c].reuse_sf) {
> > + const int blocks_per_frame =
> > s->samples_per_frame/s->subframe_len; + const int
> > res_blocks_per_frame = s->samples_per_frame / +
> > s->channel[c].scale_factor_block_len;
> >
> > + const int idx0 = av_log2(blocks_per_frame);
> > + const int idx1 = av_log2(res_blocks_per_frame);
>
> i assume these are exact 2^x values in which case maybe storing them as
> log2 values might be simpler?
>
Changed.
> > + const int8_t* sf_offsets = s->sf_offsets[idx0][idx1];
> > + int b;
> > + for (b = 0; b < s->num_bands; b++)
> > + s->channel[c].scale_factors[b] =
> > +
> > s->channel[c].saved_scale_factors[*sf_offsets++]; + }
> > +
> >
> > + if (s->channel[c].cur_subframe > 0) {
> > + s->channel[c].transmit_sf = get_bits1(&s->gb);
> > + } else
> > + s->channel[c].transmit_sf = 1;
> > +
> > + if (s->channel[c].transmit_sf) {
>
> can transmit_sf be a local var here? theres no other use in this patch
>
Changed.
> > + if (s->channel[c].max_scale_factor < *sf)
> > + s->channel[c].max_scale_factor = *sf;
>
> FFMAX
>
Changed.
>
> > + memset(s->channel[c].coeffs, 0, sizeof(float) * subframe_len);
>
> sizeof(*s->channel[c].coeffs)
>
Changed.
> > +/**
> > + *@brief Decode one WMA frame.
> > + *@param s codec context
> > + *@return 0 if the trailer bit indicates that this is the last frame,
> > + * 1 if there are additional frames
> > + */
> > +static int decode_frame(WMA3DecodeContext *s)
> > +{
> > + GetBitContext* gb = &s->gb;
> > + int more_frames = 0;
> > + int len = 0;
> > + int i;
> > +
> > + /** check for potential output buffer overflow */
> > + if (s->samples + s->num_channels * s->samples_per_frame >
> > s->samples_end) {
>
> can overflow
> s->samples_end - s->samples < ...
> should be better
>
Fixed.
> > + /** decode all subframes */
> > + while (!s->parsed_all_subframes) {
> > + if (decode_subframe(s) < 0) {
> > + s->packet_loss = 1;
> > + return 0;
> > + }
> > + }
>
> are all the previous frames to a failed frames used or droped?
> i think droping all might not be wise but maybe the last few should
> be droped (depends on which works best)
>
All previous frames are outputed. Only the broken frame and frames following
it in the same packet are dropped.
> > +
> > + /** interleave samples and write them to the output buffer */
> > + for (i = 0; i < s->num_channels; i++) {
> > + float* ptr;
> > + int incr = s->num_channels;
> > + float* iptr = s->channel[i].out;
> > + int x;
> > +
> > + ptr = s->samples + i;
> > +
> > + for (x = 0; x < s->samples_per_frame; x++) {
> > + *ptr = av_clipf(*iptr++, -1.0, 32767.0 / 32768.0);
> > + ptr += incr;
> > + }
> > +
> >
> > + /** reuse second half of the IMDCT output for the next frame */
> > + memmove(&s->channel[i].out[0],
> > + &s->channel[i].out[s->samples_per_frame],
> > + s->samples_per_frame * sizeof(float));
>
> doesnt look like it needs a move besides are you sure that cannot be
> avoided?
>
Changed. I don't think the copy can be avoided, though.
> [...]
>
> > +/**
> > + *@brief Fill the bit reservoir with a (partial) frame.
> > + *@param s codec context
> > + *@param gb bitstream reader context
> > + *@param len length of the partial frame
> > + *@param append decides wether to reset the buffer or not
> > + */
> > +static void save_bits(WMA3DecodeContext *s, GetBitContext* gb, int len,
> > + int append)
> > +{
> > + int buflen;
> > + int bit_offset;
> > + int pos;
> > +
> > + if (!append) {
> > + s->frame_offset = get_bits_count(gb) & 7;
> > + s->num_saved_bits = s->frame_offset;
> > + }
> > +
> > + buflen = (s->num_saved_bits + len + 8) >> 3;
> > +
> > + if (len <= 0 || buflen > MAX_FRAMESIZE) {
> > + av_log_ask_for_sample(s->avctx, "input buffer too small\n");
> > + s->packet_loss = 1;
> > + return;
> > + }
> > +
> > + if (!append) {
> > + s->num_saved_bits += len;
> > + memcpy(s->frame_data, gb->buffer + (get_bits_count(gb) >> 3),
> > + (s->num_saved_bits + 8)>> 3);
> > + skip_bits_long(gb, len);
> > + } else {
> > + bit_offset = s->num_saved_bits & 7;
> > + pos = (s->num_saved_bits - bit_offset) >> 3;
> > +
> > + s->num_saved_bits += len;
> > +
> > + /** byte align prev_frame buffer */
> > + if (bit_offset) {
> > + int missing = 8 - bit_offset;
> > + missing = FFMIN(len, missing);
> > + s->frame_data[pos++] |=
> > + get_bits(gb, missing) << (8 - bit_offset - missing);
> > + len -= missing;
> > + }
> > +
> > + /** copy full bytes */
> > + while (len > 7) {
> > + s->frame_data[pos++] = get_bits(gb, 8);
> > + len -= 8;
> > + }
> > +
> > + /** copy remaining bits */
> > + if (len > 0)
> > + s->frame_data[pos++] = get_bits(gb, len) << (8 - len);
> > +
> > + }
> > +
> > + init_get_bits(&s->gb, s->frame_data, s->num_saved_bits);
> > + skip_bits(&s->gb, s->frame_offset);
>
> ff_copy_bits()
> also you could keep a PutBitContext instead of dealing with remaining %8
> bits
Changed.
> and maybe some frames can be decoded without copying them
>
Some frames can indeed be decoded without copying them but broken files might
then cause reads outside the buffer boundaries.
Regards
Sascha
-------------- next part --------------
A non-text attachment was scrubbed...
Name: wmapro_try6.patch
Type: text/x-diff
Size: 53098 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090829/34c384c5/attachment.patch>
More information about the ffmpeg-devel
mailing list