[FFmpeg-devel] [PATCH] lavfi: reimplement MPlayer's af_pan filter for libavfilter.

[Nicolas George]

Le tridi 23 brumaire, an CCXX, Stefano Sabatini a écrit :
> this is used just once, please skip the obfuscating wrapper

Ok.

> you may add a comment here explaining what contains nb_named[0] and
> nb_named[1]

Ok.

> ...does not exist in the chosen output layout

Good.

> > +            out_ch_id = get_channel_number(pan->out_channels_layout, out_ch_id);
> uh? If my understanding is correct, this is a number of channels, so
> maybe nb_channels or n may be better, the recursive definition and the
> ambiguity on the out_ch_id variable is rather confusing.

There is a trick here. out_ch_id is the index/identity of the requested
output channel, either amongst all the possible output channels if named==1
or amongst the channels of the selected output layout if named==0.

In the end, we need the second, so if named==1, we convert, and here is the
trick, on an example:

	0b00110011 = out_channel_layout (FL+FR+BL+BR = quad)
	             out_ch_id = 4 = BL
	0b00010000 = 1 << out_ch_id
	0b00001111 = (1 << out_ch_id) - 1
	0b00000011 = out_channel_layout & ((1 << out_ch_id) - 1)

Thus, we are counting the number of channels in out_channel_layout that come
before out_ch_id, which is exactly the index of out_ch_id.

I added a comment, not as verbose as above, to explain a bit more.

> > +            if (sscanf(arg, " %lf %n* %n", &gain, &len, &len))
> Possibly dumb question: are the spaces really required? I mean why not
> "%lf%n*%n"?

They serve the same purpose as skip_spaces: allow to write
"FL = 0.5 * FL + 0.5 * BL" instead of cramming all together.

> Nit: !named should be more clear

Ok.

> > +        if (t > -1E-5 && t < 1E-5) {
> > +            if (t)
> > +                av_log(ctx, AV_LOG_WARNING,
> > +                       "Degenerate coefficients while renormalizing\n");
> > +            continue;
> > +        }
> Could you comment on this? What does this condition represent?

To renormalise the coefficients for an output channel, we divide them all by
their sum, so we need that the sum is not 0, and even better: not almost 0.

There are three cases where the sum, t, can be 0:

- if the user requested a silent channel: normal, leave it that way, and t
  will be exactly 0;

- if the user defined a channel using only non-existing channels: for
  example "BC<BL+BR" while the input has no back channels: still normal, and
  t still exactly 0;

- the user used positive and negative coefficients, the total happens to be
  0: probably an error, and t possibly not exactly 0.

Therefore, if t is almost 0 but not exactly 0, there is probably something
wrong.

A more reliable way to issue the warning would be to check if there are
non-0 coefficients, but that would require additional code, while this
warning is almost free, since we need to check if t is almost 0 to avoid
silly divisions.

> nit+++: (int16_t *) is more customary

Changed. That came from the old code I believe.

I also changed out_channels_layout into out_channel_layout, to be coherent
with the rest of the spelling. New patch incoming.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20111116/3bd4ceca/attachment.asc>