[FFmpeg-soc] [soc] Questions on patch order, threads and review comments

[Stefano Sabatini]

On date Tuesday 2010-07-06 02:45:09 -0700, S.N. Hemanth Meenakshisundaram encoded:
> 
> Hi Stefano,
> 
> I have a few questions on the order of patches and on some of your review
> comments from earlier:
> 
> >> >
> >> >Here is the working af_resample.c and associated Makefile and
> >> >allfilters.c changes.
> >> >
> >> >[...]
> >>
> >>
> >
> >> -#define AV_PERM_READ     0x01   ///< can read from the buffer
> >> -#define AV_PERM_WRITE    0x02   ///< can write to the buffer
> >> -#define AV_PERM_PRESERVE 0x04   ///< nobody else can overwrite the
> >> buffer
> >> -#define AV_PERM_REUSE    0x08   ///< can output the buffer multiple
> >> times, with the same contents each time
> >> -#define AV_PERM_REUSE2   0x10   ///< can output the buffer multiple
> >> times, modified each time
> >
> > The AV_PERM_* moving can go to a separate patch.
> 
> Should I submit the AV_PERM_* patch first (move it out of AVFilterPicRef
> structure) and then the the audio framework changes patch? Or should I
> focus on the audio filter patches and define the PERM macros twice (inside
> AVFilterPicRef and AVFilterSamplesRef structures).

Possibly to a separate change chronologically done *before* the audio
framework patch. The objective is to make the audio framework patch
smaller and simplify review. Also the first one should be pretty easy
and pretty fast to get applied.

Note that with git that shouldn't be at all an issue, you can work out
the patches in a branch, change their position, merge and split them
using git rebase -i, and no-one is preventing you to send many patches
in a branch, so they can start to be discussed even when the
pre-requisite patches (like this one) have not yet been applied.

> >
> >> -    link->format = link->in_formats->formats[0];
> >> +    if (link->type == AVMEDIA_TYPE_VIDEO)
> >> +        link->format = link->in_formats->formats[0];
> >> +    else if (link->type == AVMEDIA_TYPE_AUDIO)
> >> +        link->aformat = link->in_formats->aformats[0];
> >
> > simpler: link->format = type == VIDEO ? link->in_formats->formats[0] :
> > link->in_formats->aformats[0];
> 
> Here I need to assign to link->format in case of video and link->aformat
> in case of audio. How can I get this done using the ?: form above?

Forget about that, my suggestion was just wrong.
And you can put them in a single line to help readability if you
prefer it:

    if      (link->type == AVMEDIA_TYPE_VIDEO) link->format  = link->in_formats->formats[0];
    else if (link->type == AVMEDIA_TYPE_AUDIO) link->aformat = link->in_formats->aformats[0];

> >
> >> +#define CMP_AND_ADD(acount, bcount, afmt, bfmt, retfmt) { \
> >> +    for(i = 0; i < acount; i++) \
> >> +        for(j = 0; j < bcount; j++) \
> >> +            if(afmt[i] == bfmt[j]) \
> >> +                retfmt[k++] = afmt[i]; \
> >> +}
> >> [...]
> >>
> >
> > This refactoring is nice but please split it in a separate patch (and
> > separate thread).
> 
> Again, should I submit the CMP_AND_ADD refactoring patch first before the
> audio filter patches?

You can send them *before* or *togheter* as you see fit.
 
> I should start a new thread for each patch - is that right?

Possibly, that help the reviewer/committer to understand if a
*specific* patch has been already approved / committed, better than
browse through all the mails in a super-long thread.
 
> Also, I now have some patches based on your comments and some of the
> additions for channel layout conversion etc.
> 
> Should I send these as patches of patches or send the entire patch each
> time starting from whatever is not yet in the soc svns? Patches of patches
> would be difficult to read while sending the entire patch again hides the
> new changes.

Patches as issued by git should be fine to post, and the number in the
patch help to clarify the dependencies of the patch.

> Finally, Wikipedia says the downmix formulae for ac3 to stereo are:
> 
> Left Only = Left (Front) + -3dB*Center + att*LeftSide
> 
> Right Only = Right (Front) + -3dB*Center + att*RightSide
> 
> where att = -3dB, -6dB or -9dB.
> 
> The article has no references. Can I just go ahead and use this? It seems
> to work well for a 5.0 track I have with att = -9dB.

If it works and some audio-competent reviewer approve it should be
safe, anyway a reference in the code to the original article would be
nice / useful.

Regards.