[FFmpeg-devel] [PATCH 02/10] lavfi: support unknown channel layouts.

[Nicolas George]

Thanks for the review. I will answer immediately to the points that will
make reading the code easier.

Le duodi 12 nivôse, an CCXXI, Stefano Sabatini a écrit :
> > -        if (!link->in_channel_layouts->nb_channel_layouts) {
> > +        if (link->in_channel_layouts->all_layouts) {
> is this logic still valid? It looks backward to me.

The old logic was: empty list means all layouts. Therefore, with the
additional field, !nb_channel_layouts is equivalent to all_layouts. With the
support of unknown layouts, !nb_channel_layouts would be replaced either by
all_layouts or all_counts, depending on the exact situation (all_counts
implies all_layouts, it is ensured).

> > +    if (a_all < b_all) {
> > +        FFSWAP(AVFilterChannelLayouts *, a, b);
> > +        FFSWAP(unsigned, a_all, b_all);
> > +    }
> please add a comment here to clarify the logic

In case it was not clear after reading: the point of this block is to add
the more generic set in a and the other in b, so that the comparison
afterwards can be made only one way.

> > +    /* a[known] inter b[known] */
> nit: inter -> intersect

I realize I am very unfamiliar with the English vocabulary for set theory.
How do you read the \cap / U+2229 INTERSECTION symbol?

> since we suppose that known channels are stored before the unknown
> channels, couldn't you just break here?

Actually, I had dropped that constraint; I must have forgotten to remove it
from the doxy. It does not make the code any simpler, and it may later be
broken by the reduce function.

> 
> > +    /* a[known] inter b[generic] + a[generic] inter b[known] */
> > +    for (round = 0; round < 2; round++) {
> > +        for (i = 0; i < a->nb_channel_layouts; i++) {
> > +            uint64_t fmt = a->channel_layouts[i], bfmt;
> > +            if (!fmt || !KNOWN(fmt))
> > +                continue;
> > +            bfmt = FF_CHAN2LAYOUT(av_get_channel_layout_nb_channels(fmt));
> > +            for (j = 0; j < b->nb_channel_layouts; j++)
> > +                if (b->channel_layouts[j] == bfmt)
> > +                    ret->channel_layouts[ret_nb++] = a->channel_layouts[i];
> > +        }
> > +        FFSWAP(AVFilterChannelLayouts *, a, b);
> > +    }

For reference, this block works like that: do something asymmetric between a
and b, swap a and b, do it again, and swap a and b back. The result is that
the asymmetric something is done in each direction.

> I have the feeling that the code is overcomplicated to perform the
> merge and I'm having an hard time at grabbing the logic.
> 
> I suppose it may be as simple as:
> - merge known layouts
> - merge counts
> - normalize

I am not sure what normalize covers, but I am afraid you forget one thing:
this is not just a set intersection, because channel counts can also be
matched with channel layouts. For example:

merge( { STEREO, 5.1 } , { 1_ch, 2_ch } ) = { STEREO }

because STEREO and 2_ch match each-other (I can not imagine a filter that
can handle unknown layouts but not known ones with the same number of
channels).

> Maybe storing layouts and counts in separate fields may simplify the
> iterative logic.

I tried that. The loops look mostly the same, and the rest of the code,
especially the reduce and pick parts, become quite awkward, because they
iterate over the whole set without much caring what kind of elements they
are dealing with.

> CHANNELS2LAYOUT?
> LAYOUT2CHANNELS?

I would like to keep the name not too long, if that is all right.

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/20130101/c17b27d4/attachment.asc>