Quoting Diego Biurrun (2015-02-25 15:28:14)
> From: Niels Möller <[email protected]>
>
> ---
> Review questions from last round addressed.
>
The commit message and the changelog still claim it's lossless. They
should both clearly say that it's not full support.
> +
> + if (chset->downmix_coeff_code_embedded) {
> + /* nDownmixCoeffs is specified as N * M. For a primary
> + * channel set, it appears that N = number of
> + * channels, and M is the number of downmix channels.
> + *
> + * For a non-primary channel set, N is specified as
> + * number of channels + 1, and M is derived from the
> + * channel set hierarchy, and at least in simple cases
> + * M is the number of channels in preceeding channel
> + * sets. */
> + if (chset->primary_ch_set) {
> + static const char dmix_table[7] = { 1, 2, 2, 3, 3, 4, 4
> };
> + chset->downmix_ncoeffs = chset->channels *
> dmix_table[chset->downmix_type];
> + } else
> + chset->downmix_ncoeffs = (chset->channels + 1) *
> s->xll_channels;
> +
> +
> + if (chset->downmix_ncoeffs > DCA_XLL_DMIX_NCOEFFS_MAX) {
> + av_log(s->avctx, AV_LOG_WARNING,
> + "XLL: Skipping %d downmix coefficients, exceeding
> impl. limit %d\n",
> + chset->downmix_ncoeffs, DCA_XLL_DMIX_NCOEFFS_MAX);
> + skip_bits_long(&s->gb, 9 * chset->downmix_ncoeffs);
> + chset->downmix_ncoeffs = 0;
> + } else {
> + if (chset->primary_ch_set) {
> + for (i = 0; i < chset->downmix_ncoeffs; i++)
> + if ((chset->downmix_coeffs[i] =
> dca_get_dmix_coeff(s)) == -1)
You're checking for -1, but the function returns a proper error code.
Same below twice.
> + return AVERROR_INVALIDDATA;
> + } else {
> + unsigned c, r;
> + for (c = i = 0; c < s->xll_channels; c++, i +=
> chset->channels + 1) {
> + if ((chset->downmix_coeffs[i] =
> dca_get_inv_dmix_coeff(s)) == -1)
> + return AVERROR_INVALIDDATA;
> + for (r = 1; r <= chset->channels; r++) {
> + int32_t coeff = dca_get_dmix_coeff(s);
> + if (coeff == -1)
> + return AVERROR_INVALIDDATA;
> + chset->downmix_coeffs[i + r] =
> + (chset->downmix_coeffs[i] * (int64_t)
> coeff + (1 << 15)) >> 16;
Maybe I'm just missing something, but seems to me this can overflow the
array.
> +
> + data_start = get_bits_count(&s->gb);
> + if (data_start + 8 * s->xll_navi.band_size[0] > asset_end) {
> + av_log(s->avctx, AV_LOG_ERROR,
> + "XLL: Data in NAVI table exceeds containing asset\n"
> + "start: %d (bit), size %u (bytes), end %d (bit), error %u\n",
> + data_start, s->xll_navi.band_size[0], asset_end,
> + data_start + 8 * s->xll_navi.band_size[0] - asset_end);
So shouldn't this return an error?
> +
> + for (chset_i = in_channel = 0; chset_i < s->xll_nch_sets; chset_i++)
> {
> + /* The spec isn't very explicit, but I think the NAVI sizes are
> in bytes. */
> + int end_pos = get_bits_count(gb) +
> + 8 * s->xll_navi.chset_size[0][seg][chset_i];
> + int i, j;
> + struct coding_params *params = ¶m_state[chset_i];
> + /* I think this flag means that we should keep seg_type and
> + * other parameters from the previous segment. */
> + int use_seg_state_code_param;
> + XllChSetSubHeader *chset = &s->xll_chsets[chset_i];
> + if (in_channel >= s->avctx->channels)
> + /* FIXME: Could go directly to next segment */
> + goto next_chset;
> +
> + if (s->avctx->sample_rate != chset->sampling_frequency) {
> + av_log(s->avctx, AV_LOG_WARNING,
> + "XLL: unexected chset sample rate %d, expected %d\n",
nit: unexpected
> @@ -1050,8 +1159,19 @@ static int dca_decode_frame(AVCodecContext *avctx,
> void *data,
> DCAContext *s = avctx->priv_data;
> int channels, full_channels;
> int core_ss_end;
> + int upsample = 0;
> +
> + if (!avpkt->pos)
> + s->frame_index =
> + s->sample_index = 0;
>
> - s->xch_present = 0;
> + av_log(avctx, AV_LOG_DEBUG,
> + "%s: stream_index %d, pos %"PRId64", frame index %u, sample index
> %u\n",
> + __func__, avpkt->stream_index, avpkt->pos,
> + s->frame_index, s->sample_index);
> + s->frame_index++;
I think this debugging log should be dropped, along with
frame_index/sample_index.
--
Anton Khirnov
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
