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

[Stefano Sabatini]

On date Tuesday 2013-01-01 23:44:09 +0100, Nicolas George encoded:
> 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).

Alright.

> > > +    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.

Yes I realized that after reading, a simple comment like:
|swap a and b to make sure a is the more generic set

> 
> > > +    /* 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?

I suppose "intersect" should be fine, as "intersection" is the usual
name of the operation.

> 
> > 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.

OK.
 
> > 
> > > +    /* 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.

Allright, LAYOUT2CHANS (with the ending S) may be a good compromise.

Thanks for the clarifications.
-- 
FFmpeg = Fantastic Fierce Mastering Puristic Enlightened Gymnast