Thanks Amanda (and Stephen :-)... I finally got to look at this code in the train this morning (after a very relaxing extended long weekend :-)... And I have an idea of how we could do this more easily and hopefully safe enough...
I don't think I can do the save/restore trick in PlatformDeviceMac::LoadClippingRegionToCGContext(), because this a static method with no context... But I noticed that it is only called at one place, within BitmapPlatformDeviceMac::BitmapPlatformDeviceMacData::LoadConfig() which is also the only place where LoadTransformToCGContext() is called, so we could simply rely on restore/save there... But is it safe enough? The other alternative, would be to create an image mask in PlatformDeviceMac::LoadClippingRegionToCGContext(), and use CGContextClipToMask() instead of CGContextClipToRect()... But note that ClipToMask was only introduced in 10.4, I know that our development environment only supports 10.5 and up, but do we expect Chromium to run on versions earlier than 10.4? Thanks again! BYE MAD On Sat, May 16, 2009 at 4:55 PM, Amanda Walker <ama...@chromium.org> wrote: > The code to look at is in skia/ext/platform_device_mac.cc:123 and > following (PlatformDeviceMac::LoadClippingRegionToCGContext). It > calls CGContextClipToRect, but does not reset the clip path first, so > we we end up with the behavior Stephen described. > > The best solution would be to do a save when the device is created and > a restore / save pair before loading a new clip path. We'll have to > reload the rest of the Skia drawing state back into the CGContext > after the restore, of course, but that shouldn't be too bad. We might > also be able to define a mask that we draw through (and that we can > reset at will), rather than manipulating the CGContext's clip region > at all. > > It is possible to reset a CGContext's clip region (which matches the > Skia drawing model a bit better) using ClipCGContextToRegion, but that > was deprecated in 10.4 and won't be around for 64-bit applications, so > I'd rather not use it unless the other approaches turns out to be > impractical for some reason. > > --Amanda > > On Sat, May 16, 2009 at 3:44 PM, Marc-Andre Decoste <m...@google.com> > wrote: > > Salut Stephen, > > > > thanks for the hint... Note that I have tried calling save/restore on > the > > canvas around each sub-rect drawing... With no luck... I also tried to > make > > the call to canvas->getTopPlatformDevice().accessBitmap(false); after > every > > sub-rect... same thing... > > > > I guess I'll wait for Amanda to take a look at it tomorrow or > Monday... I > > quickly looked at the skia/ext/platform_canvas_mac.cc file content but > > didn't see anything related to clip rects in there... > > > > Thanks! > > > > BYE > > MAD > > > > On Sat, May 16, 2009 at 2:13 PM, Stephen White <senorbla...@chromium.org > > > > wrote: > >> > >> On Sat, May 16, 2009 at 10:16 AM, Marc-Andre Decoste <m...@google.com> > >> wrote: > >>> > >>> Salut, > >>> > >>> I found the cause of the problem, but didn't find a cure yet.. The > >>> SkCanvas::ClipRect() call with the replace Op doesn't seem to work more > than > >>> once... The first rect gets drawn on the cancas properly, but all > subsequent > >>> rects drawn on the same canvas with different clip rects seem to be > >>> ignored... If I don't make the call to clip the rect, then all the > rects are > >>> drawn, but some of the earlier rects can be over-painted with white > when > >>> other rects are drawn at the same height within the canvas... > >>> > >>> I tried to clear the clip rect after drawing each rectagle (using > the > >>> kDifference_Op), also tried unioning the clip rect before doing an > intersect > >>> op, still with no success... I'll keep digging to understand why the > >>> subsequent clip rects don't work on the Mac when they work fine on > Windows > >>> and Linux... But I would appreciate some hints if you have some... :-) > >> > >> I haven't looked at the code, but this sounds a lot like a peculiarity > of > >> CoreGraphics I ran into > >> a while back: setting the clip path with CGContextClipToRect() does the > >> intersection of that > >> rect with the existing clip path, rather than replacing it. Since the > >> clip path can't be made > >> larger, the only way to really restore it is to save the graphics state > >> before setting the clip > >> path, and restore the graphics state after drawing. So if we're > not doing > >> that, it's possible that > >> the CoreGraphics path may not have the same semantics as the Skia path > in > >> this case. > >> (It's my understanding that we use CoreGraphics on the Mac rather than > >> Skia, > >> but since you mentioned SkCanvas, I could be wrong.) > >> Stephen > >> > >>> > >>> On Wed, May 13, 2009 at 1:25 PM, Marc-Andre Decoste <m...@google.com> > >>> wrote: > >>>> > >>>> As I mentioned in the latest message, this code is OK after all :-)... > >>>> The problem seems to be at the other end, when I split the drawing in > >>>> sub-regions of the TranportDIB, on the rendering side... I wasn't > expecting > >>>> this, since this other code is not using any platform specific stuff > >>>> directly... You can find it in here: > http://codereview.chromium.org/108040 > >>>> The problem I get is that I'm missing some refreshes... I have a > little > >>>> test HTML file that changes the content on of multiple parts of the > page as > >>>> I press on buttons, and it doesn't work until I change the code > >>>> in chrome/renderer/render_widget.cc by calling PaintRect(bitmap_rect, > >>>> canvas); instead of PaintRects(paint_rects, canvas);, which means we > draw > >>>> the whole bitmap, even though we ask the host to only paint a set of > >>>> sub-rectangle from that bitmap. > >>>> Do I explain it clearly enough? > >>>> Thanks > >>>> BYE > >>>> MAD > >>>> On Wed, May 13, 2009 at 1:16 PM, Amanda Walker <awal...@google.com> > >>>> wrote: > >>>>> > >>>>> Hmm. Marc, your change looks OK to me at first glance--how does it > >>>>> fail? > >>>>> > >>>>> --Amanda > >>>>> > >>>>> On Wed, May 13, 2009 at 12:10 PM, Avi Drissman <a...@google.com> > wrote: > >>>>> > I'm view meister, not render meister. Amanda is IIRC backing > >>>>> > store/canvas > >>>>> > meister. > >>>>> > > >>>>> > Avi > >>>>> > > >>>>> > On Wed, May 13, 2009 at 9:04 AM, Marc-Andre Decoste < > m...@google.com> > >>>>> > wrote: > >>>>> >> > >>>>> >> OK, so it seems this code is fine.... > >>>>> >> I tried to make a temporary change in the other (platform > >>>>> >> independent) > >>>>> >> part of the code, where we paint the sub-rects into a bigger > >>>>> >> bitmap... I > >>>>> >> thought it would work without any changes on the Mac side since it > >>>>> >> was all > >>>>> >> done with platform independent calls, but maybe a platform > dependent > >>>>> >> implementation on the other side of that call doesn't like it.. > >>>>> >> So I'll start digging in this other direction... But only after > >>>>> >> lunch... > >>>>> >> Will keep you guys posted of my progress... > >>>>> >> Thanks! > >>>>> >> > >>>>> >> BYE > >>>>> >> MAD > >>>>> >> > >>>>> >> On Wed, May 13, 2009 at 9:35 AM, Mike Pinkerton > >>>>> >> <pinker...@google.com> > >>>>> >> wrote: > >>>>> >>> > >>>>> >>> Looping in Avi who is our render-meister. > >>>>> >>> > >>>>> >>> On Wed, May 13, 2009 at 9:24 AM, Marc-Andre Decoste > >>>>> >>> <m...@google.com> > >>>>> >>> wrote: > >>>>> >>> > Salut Mac-Heros, > >>>>> >>> > I'm trying to update the Mac platform specific code to some > >>>>> >>> > changes > >>>>> >>> > I did > >>>>> >>> > to the Windows platform specific code in an attempt to improve > >>>>> >>> > our > >>>>> >>> > paint > >>>>> >>> > invalidation performance (as discussed on chromium-dev@). > >>>>> >>> > Below a patch of my first (unsuccessful attempt). I must > admit > >>>>> >>> > that > >>>>> >>> > even > >>>>> >>> > on Windows it took me a few iterations to get it right, so I > >>>>> >>> > would have > >>>>> >>> > been > >>>>> >>> > surprised to get it right on the first try on the Mac... > >>>>> >>> > So the idea is that we now receive a bigger bitmap that > >>>>> >>> > contains > >>>>> >>> > sub-rectangles that we want to use to redraw individually... So > I > >>>>> >>> > thought > >>>>> >>> > that this was the goal of drawBitmapRect() but I guess I > >>>>> >>> > misunderstood > >>>>> >>> > it's > >>>>> >>> > usage... Are you guys familiar with this? Anybody else I could > >>>>> >>> > ask > >>>>> >>> > questions > >>>>> >>> > to about this? > >>>>> >>> > In the meant time, I'm also working with the linux version > and > >>>>> >>> > attempting > >>>>> >>> > different shots in the dark on the Mac :-)... > >>>>> >>> > Thanks! > >>>>> >>> > BYE > >>>>> >>> > MAD > >>>>> >>> > Index: chrome/browser/renderer_host/backing_store_mac.cc > >>>>> >>> > > >>>>> >>> > > =================================================================== > >>>>> >>> > --- chrome/browser/renderer_host/backing_store_mac.cc (revision > >>>>> >>> > 15881) > >>>>> >>> > +++ chrome/browser/renderer_host/backing_store_mac.cc (working > >>>>> >>> > copy) > >>>>> >>> > @@ -21,14 +21,21 @@ > >>>>> >>> > > >>>>> >>> > void BackingStore::PaintRect(base::ProcessHandle process, > >>>>> >>> > TransportDIB* bitmap, > >>>>> >>> > - const gfx::Rect& bitmap_rect) { > >>>>> >>> > + const gfx::Rect& bitmap_rect, > >>>>> >>> > + const gfx::Rect& paint_rect) { > >>>>> >>> > SkBitmap skbitmap; > >>>>> >>> > skbitmap.setConfig(SkBitmap::kARGB_8888_Config, > >>>>> >>> > bitmap_rect.width(), > >>>>> >>> > bitmap_rect.height(), 4 * > >>>>> >>> > bitmap_rect.width()); > >>>>> >>> > > >>>>> >>> > skbitmap.setPixels(bitmap->memory()); > >>>>> >>> > - > >>>>> >>> > - canvas_.drawBitmap(skbitmap, bitmap_rect.x(), > >>>>> >>> > bitmap_rect.y()); > >>>>> >>> > + SkIRect src_rect; > >>>>> >>> > + src_rect.set(paint_rect.x(), paint_rect.y(), > >>>>> >>> > + paint_rect.right(), paint_rect.bottom()); > >>>>> >>> > + src_rect.offset(-bitmap_rect.x(), -bitmap_rect.y()); > >>>>> >>> > + SkRect dst_rect; > >>>>> >>> > + dst_rect.iset(paint_rect.x(), paint_rect.y(), > >>>>> >>> > + paint_rect.right(), paint_rect.bottom()); > >>>>> >>> > + canvas_.drawBitmapRect(skbitmap, &src_rect, dst_rect); > >>>>> >>> > } > >>>>> >>> > > >>>>> >>> > void BackingStore::ScrollRect(base::ProcessHandle process, > >>>>> >>> > @@ -117,6 +124,6 @@ > >>>>> >>> > } > >>>>> >>> > > >>>>> >>> > // Now paint the new bitmap data. > >>>>> >>> > - PaintRect(process, bitmap, bitmap_rect); > >>>>> >>> > + PaintRect(process, bitmap, bitmap_rect, bitmap_rect); > >>>>> >>> > return; > >>>>> >>> > > >>>>> >>> > >>>>> >>> > >>>>> >>> > >>>>> >>> -- > >>>>> >>> Mike Pinkerton > >>>>> >>> Mac Weenie > >>>>> >>> pinker...@google.com > >>>>> >> > >>>>> > > >>>>> > > >>>> > >>> > >>> > >>> > >> > >> > >> > >> -- > >> All truth passes through three stages. First, it is ridiculed. Second, > it > >> is violently opposed. Third, it is accepted as being self-evident. -- > >> Schopenhauer > > > > > > > > > > > --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
