>L34/R34
Calculating the 3/4 power of the coefficients isn't really that expensive
to do twice, so I'll just scrap the whole idea.
>identation
How'd I miss that? Fixed.
>phase
Copied that from an older revision, missed that '*' sign and didn't notice,
fixed.
Will wait a while to see whether the other commits look good before sending
the fixed one.
On 4 August 2015 at 08:31, Claudio Freire <klaussfre...@gmail.com> wrote:
> On Wed, Jul 29, 2015 at 1:44 AM, Rostislav Pehlivanov
> <atomnu...@gmail.com> wrote:
> > + if (cpe->ms_mode)
> > + phase = 1 - 2 * cpe->ms_mask[w*16+g];
>
>
> Shouldn't it be ?
>
> phase *= 1 - ... ;
>
> phase is an argument, the original code would step on it, with a value
> that doesn't depend on phase, so it would fail to evaluate both
> phases. Using phase *= would make sure to test both phases.
>
> Well, that's the general idea, except it breaks the phase assigned to
> the struct. Something like the following does work though:
>
> ephase = phase;
> if (cpe->ms_mode)
> ephase *= 1 - 2 * cpe->ms_mask[w*16+g];
>
> and then change all uses of phase into ephase, except the last that
> remains:
>
> is_error.phase = phase; // original phase
> is_error.pass = dist2 <= dist1;
>
>
> On Wed, Jul 29, 2015 at 1:44 AM, Rostislav Pehlivanov
> <atomnu...@gmail.com> wrote:
> > for (w = 0; w < 128; w++)
> > if (sce1->band_type[w] >= INTENSITY_BT2)
> > sce1->band_type[w] = 0;
> >
> > - if (!cpe->common_window)
> > - return;
> > - for (w = 0; w < sce0->ics.num_windows; w += sce0->ics.group_len[w])
> {
> > - start = 0;
> > - for (g = 0; g < sce0->ics.num_swb; g++) {
> > - if (start*freq_mult > INT_STEREO_LOW_LIMIT*(lambda/170.0f)
> &&
> > - cpe->ch[0].band_type[w*16+g] != NOISE_BT &&
> !cpe->ch[0].zeroes[w*16+g] &&
> > - cpe->ch[1].band_type[w*16+g] != NOISE_BT &&
> !cpe->ch[1].zeroes[w*16+g]) {
> > - int phase = 0;
> > - float ener0 = 0.0f, ener1 = 0.0f, ener01 = 0.0f;
> > - float dist1 = 0.0f, dist2 = 0.0f;
> > + if (!cpe->common_window)
> > + return;
> > + for (w = 0; w < sce0->ics.num_windows; w +=
> sce0->ics.group_len[w]) {
> > + start = 0;
> > + for (g = 0; g < sce0->ics.num_swb; g++) {
> > + if (start*freq_mult >
> INT_STEREO_LOW_LIMIT*(s->lambda/170.0f) &&
>
> This looks strange. As it is that patch, it ends up with code like:
>
> > for (w = 0; w < 128; w++)
> > if (sce1->band_type[w] >= INTENSITY_BT2)
> > sce1->band_type[w] = 0;
> >
> > if (!cpe->common_window)
> > return;
> > for (w = 0; w < sce0->ics.num_windows; w +=
> sce0->ics.group_len[w]) {
> > start = 0;
> > for (g = 0; g < sce0->ics.num_swb; g++) {
>
> Which looks wrong. Bad indentation right?
>
> I think you meant:
>
> for (w = 0; w < 128; w++)
> if (sce1->band_type[w] >= INTENSITY_BT2)
> sce1->band_type[w] = 0;
>
> if (!cpe->common_window)
> return;
> for (w = 0; w < sce0->ics.num_windows; w += sce0->ics.group_len[w]) {
> start = 0;
> for (g = 0; g < sce0->ics.num_swb; g++) {
>
> A big part of the diff in that hunk is reindent, so I believe if you
> fix that indentation snafu the patch will shrink.
>
> On Wed, Jul 29, 2015 at 1:44 AM, Rostislav Pehlivanov
> <atomnu...@gmail.com> wrote:
> > for (w2 = 0; w2 < sce0->ics.group_len[w]; w2++) {
> > + abs_pow34_v(L34, sce0->coeffs+start+(w+w2)*128,
> sce0->ics.swb_sizes[g]);
> > + abs_pow34_v(R34, sce1->coeffs+start+(w+w2)*128,
> sce0->ics.swb_sizes[g]);
> > for (i = 0; i < sce0->ics.swb_sizes[g]; i++) {
> > float coef0 = sce0->pcoeffs[start+(w+w2)*128+i];
> > float coef1 = sce1->pcoeffs[start+(w+w2)*128+i];
> > - phase += coef0*coef1 >= 0.0f ? 1 : -1;
> > ener0 += coef0*coef0;
> > ener1 += coef1*coef1;
> > ener01 += (coef0 + coef1)*(coef0 + coef1);
> > }
> > }
>
> Careful, you're stepping on L34 and R34 on eight_short_window, and
> passing the last results only to calc_encoding_err_is.
>
> In fact, I'm thinking I may have induced you to make that mistake when
> I suggested not to compute R34 / L34 twice (once for each phase),
> since L34 and R34 only have room for one window, and
> calc_encoding_err_is needs to process a whole window group.
>
> I think you'll have to move it back to inside calc_encoing_err_is and
> just compute it twice. Redundant, but at least it's correct.
>
> Also, you should use pcoeffs (coeffs will have M/S applied to it when
> ms_mask).
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
