[Ffmpeg-devel] [PATCH] DTS decoder
Kostya
kostya.shishkov
Fri Feb 23 06:43:41 CET 2007
On Fri, Feb 23, 2007 at 12:06:18AM +0100, Michael Niedermayer wrote:
> Hi
>
> On Wed, Feb 21, 2007 at 01:01:13PM +0100, Benjamin Larsson wrote:
> > This is the current working version of the DCA decoder. It is mostly a
> > port of libdts/libdca to ffmpeg, Kostya have replaced all the GPL
> > downmixing code and ported the huffman stuff plus some other various
> > things, I played abit with the transform and the initial lavc port. The
> > rest of the patch files can be obtained by using svn co
> > svn://svn.mplayerhq.hu/dca dca (there are over 300kb of tables and add
> > little for a review). The current code just downmix to 2 channels like
> > the libdts glue code also did. This will hopefully change in the future.
Thanks for the review, I'll change decoder to comply with these requirements.
Most of those catches come from old libdca.
[...]
>
> also remove all other unused #defines unless theres a reason to keep them
Yes, that will get rid of lots of defines.
[...]
> > #define GET_BITALLOC(gb, ba, idx) \
> > get_vlc2(gb, ba.vlc[idx].table, ba.vlc[idx].bits, ba.wrap) + ba.offset
>
> this should be a function theres no reason for it not to be, or remove
> it completely
At least that was 'always_inline' :)
> >
> > typedef struct {
> > AVCodecContext *avctx;
> > /* Frame header */
> > int frame_type; /* type of the current frame */
> > int samples_deficit; /* deficit sample count */
> > int crc_present; /* crc is present in the bitstream */
> > int sample_blocks; /* number of PCM sample blocks */
> > int frame_size; /* primary frame byte size */
> > int amode; /* audio channels arrangement */
> > int sample_rate; /* audio sampling rate */
> > int bit_rate; /* transmission bit rate */
>
> not doxygen compatible
will doxify
[...]
> > s->output = DCA_STEREO;
> > if (s->output < 0)
> > return 1;
>
> and that does what?
another case of leftovers
[...]
> > static int dca_subframe_header(DCAContext * s)
> > {
> > /* Primary audio coding side information */
> > int j, k;
> >
> > /* Subsubframe count */
> > s->subsubframes = get_bits(&s->gb, 2) + 1;
> > #ifdef DEBUG
> > av_log(s->avctx, AV_LOG_DEBUG, "subsubframes: %i\n", s->subsubframes);
> > #endif
> >
> > /* Partial subsubframe sample count */
> > s->partial_samples = get_bits(&s->gb, 3);
> > #ifdef DEBUG
> > av_log(s->avctx, AV_LOG_DEBUG, "partial samples: %i\n",
> > s->partial_samples);
> > #endif
> >
> > /* Get prediction mode for each subband */
> > for (j = 0; j < s->prim_channels; j++) {
> > for (k = 0; k < s->subband_activity[j]; k++)
> > s->prediction_mode[j][k] = get_bits(&s->gb, 1);
> > #ifdef DEBUG
> > av_log(s->avctx, AV_LOG_DEBUG, "prediction mode:");
> > for (k = 0; k < s->subband_activity[j]; k++)
> > av_log(s->avctx, AV_LOG_DEBUG, " %i", s->prediction_mode[j][k]);
> > av_log(s->avctx, AV_LOG_DEBUG, "\n");
> > #endif
>
> and please get rid of the debug code interspaced everywhere
> use #define TRACE or if needed extent the TRACE related code
In what way? Totally drop or just move into one block?
[...]
> > void dca_downmix(float *samples, int srcfmt)
>
> add ff_ prefix or static
it's all static functions
> > {
> > int i;
> > float t;
> >
> > switch (srcfmt) {
> > case DCA_MONO:
> > case DCA_CHANNEL:
> > case DCA_STEREO_TOTAL:
> > case DCA_STEREO_SUMDIFF:
> > case DCA_4F2R:
> > av_log(NULL, 0, "Not implemented!\n");
> > break;
> > case DCA_STEREO:
> > break;
> > case DCA_3F:
> > DOWNMIX_TO_STEREO(MIX_FRONT3(samples),);
> > break;
> > case DCA_2F1R:
> > DOWNMIX_TO_STEREO(MIX_REAR1(samples, i + 512),);
> > break;
> > case DCA_3F1R:
> > DOWNMIX_TO_STEREO(MIX_FRONT3(samples),
> > MIX_REAR1(samples, i + 768));
> > break;
> > case DCA_2F2R:
> > DOWNMIX_TO_STEREO(MIX_REAR2(samples, i + 512, i + 768),);
> > break;
> > case DCA_3F2R:
> > DOWNMIX_TO_STEREO(MIX_FRONT3(samples),
> > MIX_REAR2(samples, i + 768, i + 1024));
> > break;
> > }
> > }
>
> all the downmixig code of the various audio codecs should be merged ideally
Yes, but there are two codecs now which use downmixing (ac3 and this) and
none has been integrated yet.
> > }
> > }
> >
> > /* 32 subbands QMF */
> > for (k = 0; k < s->prim_channels; k++) {
> > /* static float pcm_to_double[8] =
> > {32768.0, 32768.0, 524288.0, 524288.0, 0, 8388608.0, 8388608.0};*/
> > START_TIMER //982747 vs 884090 vs 874210 vs 706959 vs 686880 vs 677672 vs 557540 vs 429267 vs 420059 (times 16350)
> > qmf_32_subbands(s, k,
> > subband_samples[k],
> > &s->samples[256 * k],
> > 3 / 2 /*pcm_to_double[s->source_pcm_res] */ ,
>
> 3/2= 1 is this intended?
No, it should be 1.5
>
>
> > 0 /*s->bias */ );
> > STOP_TIMER("qmf_32_subbands")
> > }
> >
> > /* Down mixing */
> >
> > if (s->prim_channels > dca_channels[s->output & DCA_CHANNEL_MASK]) {
> > dca_downmix(s->samples, s->amode);
> > }
>
> cant the downmixing be moved before the qmf and qmf skiped for "lost" channels?
I'll look at this
[...]
> > {
> > uint32_t tmp = 0, mrk;
> > int bits = 0;
> > int dsize = 0;
> > int i;
> > uint16_t *ssrc = (uint16_t *) src, *sdst = (uint16_t *) dst;
> >
> > mrk = AV_RB32(src);
> > switch (mrk) {
> > case DCA_MARKER_RAW_BE:
> > memcpy(dst, src, FFMIN(src_size, max_size));
>
> cant that memcpy be avoided?
No, input = frame date, output = working buffer in codec context
> [...]
> > case DCA_MARKER_14B_BE:
> > case DCA_MARKER_14B_LE:
> > for (i = 0; i < (src_size + 1) >> 1 && dsize < max_size; i++) {
> > tmp <<= 14;
> > tmp |=
> > ((mrk ==
> > DCA_MARKER_14B_BE) ? AV_RB16(src) : AV_RL16(src)) &
> > 0x3FFF;
> > src += 2;
> > bits += 14;
> > while (bits >= 8) {
> > bits -= 8;
> > *dst++ = tmp >> bits;
> > dsize++;
> > if (dsize >= max_size)
> > break;
> > }
> > }
> > if (bits) {
> > *dst++ = tmp << (8 - bits);
> > dsize++;
> > }
>
> a put_bits(14) could simplify this quite a bit ...
yes
> [...]
>
> >
> > /**
> > * Main frame decoding function
> > * FIXME add arguments
> > */
> > static int dca_decode_frame(AVCodecContext * avctx,
> > void *data, int *data_size,
> > uint8_t * buf, int buf_size)
> > {
> >
> > int i, j, k;
> > int16_t *samples = data;
> > DCAContext *s = avctx->priv_data;
> > int channels;
> >
> >
> > *data_size = 0;
>
> you should check that the buffer is large enough
BTW maybe there should be an additional field for expected data size
so only the codecs with variable frame size would explicitly check it?
> [...]
>
> > //if the AVCodecContext is missing some values, fill in the ones we have
> > if (!avctx_check(avctx)) {
> > s->flags = 2; //force stereo for now FIXME
> > avctx->sample_rate = s->sample_rate;
> > avctx->channels = channels_multi(s->flags);
> > avctx->bit_rate = s->bit_rate;
> > }
>
> hmm
> if(!avctx->sample_rate) avctx->sample_rate = s->sample_rate
> ...
>
> seems better, or even always set them no matter if they have been set or not
ok
[...]
More information about the ffmpeg-devel
mailing list