[Mesa-dev] [PATCH 0/11] Post-processing infrastructure / gsoc work

[Brian Paul]

On 08/17/2011 01:41 AM, Lauri Kasanen wrote:
> On Tue, 16 Aug 2011 18:44:35 -0600
> Brian Paul<brian.e.paul at gmail.com>  wrote:
>
> Hi
>
> Thanks for taking the time to read them through. Patch 07 seems still stuck in the ML moderation queue.
>
>>> The set builds on top of the three cleanup patches sent earlier; they haven't been applied to master, nor have gotten any comments.
>>
>> Maybe you could re-post those?
>
> Reposted.

Looks like Corbin pushed them.


>> How exactly does the post-processing step interface to the gallium
>> drivers?  I took a quick look but it didn't jump out at me.  Maybe you
>> could also write a documentation page to add in the docs/ directory.
>
> The PP queue is bound to the DRI context. It is init and shut down with the context, and called on swapbuffers (flush for dri2 hw drivers). EGL and other such systems can implement it similarly if desired.

I don't know if this is possible, but could the post-processor be 
constructed as another gallium driver that wraps other drivers?  For 
example, the rbug driver is a wrapper driver that intercepts most of 
the context/screen methods and then passes them through to the wrapped 
driver.

If the post-processor could be a wrapper, it would seem to be more 
flexible and easily used with any winsys or state tracker.


>> Otherwise, here's some stylistic things I noted:
>
> I was told these weren't as strict for files under their own self-contained dir (aux/pp) vs changes to existing dirs (where I did take care to fit in).

Yeah, we were pretty lax about that with the old DRI drivers, but in 
Gallium I think we'd like to be more consistent.


>> 2. The copyright notices reference Tungsten Graphics.  You should
>> probably replace that with "the authors".
>
> Will change, thanks.
>
> Will do the other changes as well.

Looks good, thanks.

I spotted one other thing.  In some places you're declaring variables 
after code. Ex:

struct pp_queue_t *
pp_init(struct pipe_screen *pscreen, const unsigned int *enabled)
{

    pp_debug("Initializing the post-processing queue.\n");

    unsigned int curpos = 0, i, tmp_req = 0;
...


Some non-gcc compilers will complain about this so it should be:

struct pp_queue_t *
pp_init(struct pipe_screen *pscreen, const unsigned int *enabled)
{
    unsigned int curpos = 0, i, tmp_req = 0;

    pp_debug("Initializing the post-processing queue.\n");
...


Thanks.

-Brian