Changes x11_util::GetRenderVisualFormat to determine the format id...

Changes x11_util::GetRenderVisualFormat to keep a cache of render
picts. This way we can support different visuals while still caching
the value.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=26615

[sky, 2009-09-18 15:34:31]

As Adam is out today could one of you guys review this?

Thanks,

  -Scott

[Evan Stade, 2009-09-18 18:20:47]

I really have no clue how expensive the XRenderFindVisualFormat call is, but if
we just need one for every window depth, why don't we cache it statically per
depth?

Also, I think Dan Erat knows about X (more than I do anyway).

[sky, 2009-09-18 20:06:53]

On 2009/09/18 18:20:47, Evan Stade wrote:
> I really have no clue how expensive the XRenderFindVisualFormat call is, but
if
> we just need one for every window depth, why don't we cache it statically per
> depth?

I've changed GetRenderVisualFormat to cache per visual/display pair.

New snapshot uploaded.

  -Scott

[Evan Stade, 2009-09-18 20:13:29]

looks good, one question:

http://codereview.chromium.org/210021/diff/4002/4004
File chrome/common/x11_util.cc (right):

http://codereview.chromium.org/210021/diff/4002/4004#newcode52
Line 52: const size_t kMaxCacheSize = 5;
why 5? have you done any testing to see how many we actually end up caching if
there is no limit?

http://codereview.chromium.org/210021/diff/4002/4004#newcode368
Line 368: if ((*i).equals(dpy, visual))
nit: i->

[sky, 2009-09-18 20:28:44]

On 2009/09/18 20:13:29, Evan Stade wrote:
> looks good, one question:
> 
> http://codereview.chromium.org/210021/diff/4002/4004
> File chrome/common/x11_util.cc (right):
> 
> http://codereview.chromium.org/210021/diff/4002/4004#newcode52
> Line 52: const size_t kMaxCacheSize = 5;
> why 5? have you done any testing to see how many we actually end up caching if
> there is no limit?

It's almost always going to be 1. We get 2 with this extra window I'm creating
with an argb visual.

> http://codereview.chromium.org/210021/diff/4002/4004#newcode368
> Line 368: if ((*i).equals(dpy, visual))
> nit: i->

Done.

New snapshot uploaded.

Thanks,

  -Scott

[Evan Stade, 2009-09-18 20:33:11]

If it expected that we will never need more than 2, can we LOG or DCHECK after
exceeding that? with that, LGTM.

[sky, 2009-09-18 20:39:14]

On 2009/09/18 20:33:11, Evan Stade wrote:
> If it expected that we will never need more than 2, can we LOG or DCHECK after
> exceeding that? with that, LGTM.

There may be conditions under which you can get more. Perhaps changing your
resolution on the fly, NX client, or something like that.

  -Scott

[Evan Stade, 2009-09-18 21:58:33]

Actually I doubt that those things would cause problems, or we'd already have
gotten bug reports on them. But if it is possible in some odd scenario to need
more than 2, it would still be nice to LOG when we evict from the cache. Given
the LRU eviction, if there actually is some situation where we need >5 different
formats, we might totally lose the effect of the cache.

(Or, just ask AGL.)

[sky, 2009-09-18 22:11:36]

How about I add a DCHECK if the size of the cache gets to kMax?

  -Scott

On Fri, Sep 18, 2009 at 2:58 PM,  <estade@chromium.org> wrote:
> Actually I doubt that those things would cause problems, or we'd already
> have
> gotten bug reports on them. But if it is possible in some odd scenario to
> need
> more than 2, it would still be nice to LOG when we evict from the cache.
> Given
> the LRU eviction, if there actually is some situation where we need >5
> different
> formats, we might totally lose the effect of the cache.
>
> (Or, just ask AGL.)
>
> http://codereview.chromium.org/210021
>

[Evan Stade, 2009-09-18 22:34:30]

sgtm

-- Evan Stade



On Fri, Sep 18, 2009 at 3:11 PM, Scott Violet <sky@chromium.org> wrote:
> How about I add a DCHECK if the size of the cache gets to kMax?
>
> =A0-Scott
>
> On Fri, Sep 18, 2009 at 2:58 PM, =A0<estade@chromium.org> wrote:
>> Actually I doubt that those things would cause problems, or we'd already
>> have
>> gotten bug reports on them. But if it is possible in some odd scenario t=
o
>> need
>> more than 2, it would still be nice to LOG when we evict from the cache.
>> Given
>> the LRU eviction, if there actually is some situation where we need >5
>> different
>> formats, we might totally lose the effect of the cache.
>>
>> (Or, just ask AGL.)
>>
>> http://codereview.chromium.org/210021
>>
>

[evanm, 2010-11-09 19:30:17]

http://codereview.chromium.org/210021/diff/5002/5004
File chrome/common/x11_util.cc (right):

http://codereview.chromium.org/210021/diff/5002/5004#newcode44
chrome/common/x11_util.cc:44: CachedPictFormats* get_cached_pict_formats() {
Why not GetCachedPictFormats?