Salut, I'm getting refresh problems trying to get this patch working on the Mac: http://codereview.chromium.org/108040. This change is about splitting the rect invalidations which used to be unioned in a single rect... We still send a single bitmap but we only paint the individual invalid rects in it and pass the paint rects in an array, besides the bitmap rect.
Since there is a change in the Windows specific code of the renderer host backing store (to draw the sub-rects of the passed bitmap), I thought my goof could have been in my adaptation of the Mac version of that code ( http://codereview.chromium.org/108040/diff/2038/3040). But it seems to be working fine if I disable the other part of that change (which is to paint only sub-rectangles in the bitmap). So if I paint a complete bitmap, and pass it to the host with a list of sub-rectagles to draw... It works... So I guess the goof is in the code that paint sub-rectangles in the bitmap... But this code works fine on Windows and is not done in a Windows specific way... So I'm kind of lost and don't know where to look... Anybody has a clue here? Here's the code that I think is causing the problem: http://codereview.chromium.org/108040/diff/2031/3022 Or in text diff directly here: Index: chrome/renderer/render_widget.cc =================================================================== --- chrome/renderer/render_widget.cc (revision 15319) +++ chrome/renderer/render_widget.cc (working copy) @@ -32,6 +32,54 @@ using WebKit::WebScreenInfo; using WebKit::WebSize; +namespace { +// A helper class to manage access to the RenderWidget::paint_rects vector. +class RectVectorHelper { + public: + explicit RectVectorHelper(std::vector<gfx::Rect>* rects) + : rects_(*rects) { + } + + bool IsDamagedRectBigEnough(const gfx::Size& min_size) const { + gfx::Rect damaged_rect; + for (size_t i = 0; i < rects_.size(); ++i) + damaged_rect = damaged_rect.Union(rects_[i]); + return damaged_rect.width() >= min_size.width() && + damaged_rect.height() >= min_size.height(); + } + + void Add(gfx::Rect rect) { + if (rect.IsEmpty()) + return; + // Look for all rects that would intersect with this new + // rect, union them and remove them from the list so that we + // have only one rect for any pixel in the invalidation. + std::vector<gfx::Rect>::iterator it = rects_.begin(); + while (it != rects_.end()) { + if (rect.Intersects(*it)) { + rect = rect.Union(*it); + it = rects_.erase(it); + } else { + ++it; + } + } + rects_.push_back(rect); + } + + bool Intersects(const gfx::Rect& rect) const { + for (size_t i = 0; i < rects_.size(); ++i) { + if (rects_[i].Intersects(rect)) + return true; + } + return false; + } + + private: + std::vector<gfx::Rect>& rects_; +}; + +} // namespace + RenderWidget::RenderWidget(RenderThreadBase* render_thread, bool activatable) : routing_id_(MSG_ROUTING_NONE), webwidget_(NULL), @@ -198,12 +246,11 @@ // an ACK if we are resized to a non-empty rect. webwidget_->Resize(new_size); if (!new_size.IsEmpty()) { - DCHECK(!paint_rect_.IsEmpty()); + DCHECK(!paint_rects_.empty()); // This should have caused an invalidation of the entire view. The damaged // rect could be larger than new_size if we are being made smaller. - DCHECK_GE(paint_rect_.width(), new_size.width()); - DCHECK_GE(paint_rect_.height(), new_size.height()); + DCHECK(RectVectorHelper(&paint_rects_).IsDamagedRectBigEnough(new_size)); // We will send the Resize_ACK flag once we paint again. set_next_paint_is_resize_ack(); @@ -307,13 +354,13 @@ webwidget_->SetFocus(false); } -void RenderWidget::PaintRect(const gfx::Rect& rect, - skia::PlatformCanvas* canvas) { +void RenderWidget::PaintThisRect(gfx::Rect rect, + skia::PlatformCanvas* canvas) { + // Make sure we don't erase previous rects in the canvas + SkRect clip_rect; + clip_rect.iset(rect.x(), rect.y(), rect.right(), rect.bottom()); + canvas->clipRect(clip_rect, SkRegion::kReplace_Op); - // Bring the canvas into the coordinate system of the paint rect. - canvas->translate(static_cast<SkScalar>(-rect.x()), - static_cast<SkScalar>(-rect.y())); - // If there is a custom background, tile it. if (!background_.empty()) { SkPaint paint; @@ -326,22 +373,42 @@ } webwidget_->Paint(canvas, rect); +} +void RenderWidget::PaintRect(const gfx::Rect& rect, + skia::PlatformCanvas* canvas) { + // Bring the canvas into the coordinate system of the paint rect. + canvas->translate(static_cast<SkScalar>(-rect.x()), + static_cast<SkScalar>(-rect.y())); + PaintThisRect(rect, canvas); + // Flush to underlying bitmap. TODO(darin): is this needed? canvas->getTopPlatformDevice().accessBitmap(false); - // Let the subclass observe this paint operations. + // Let the subclass observe this paint operation. DidPaint(); } +void RenderWidget::PaintRects(const std::vector<gfx::Rect>& rects, + skia::PlatformCanvas* canvas) { + for (size_t i = 0; i < rects.size(); ++i) + PaintThisRect(rects[i], canvas); + + // Flush to underlying bitmap. TODO(darin): is this needed? + canvas->getTopPlatformDevice().accessBitmap(false); + + // Let the subclass observe these paint operations. + DidPaint(); +} + void RenderWidget::DoDeferredPaint() { - if (!webwidget_ || paint_reply_pending() || paint_rect_.IsEmpty()) + if (!webwidget_ || paint_reply_pending() || paint_rects_.empty()) return; // When we are hidden, we want to suppress painting, but we still need to // mark this DoDeferredPaint as complete. if (is_hidden_ || size_.IsEmpty()) { - paint_rect_ = gfx::Rect(); + paint_rects_.clear(); needs_repainting_on_restore_ = true; return; } @@ -351,26 +418,34 @@ // OK, save the current paint_rect to a local since painting may cause more // invalidation. Some WebCore rendering objects only layout when painted. - gfx::Rect damaged_rect = paint_rect_; - paint_rect_ = gfx::Rect(); + std::vector<gfx::Rect> paint_rects = paint_rects_; + paint_rects_.clear(); // Compute a buffer for painting and cache it. + DCHECK(!current_paint_buf_); + + // we use the whole view size as opposed to damaged rect size we have a pool + // of paint buffers anyway, and this size has surely been used at least once + // to paint the whole view... And it also prevents, somehow, an intermittent + // bug we get when painting the sub-rectangles with StretchDIBits on Windows. + gfx::Rect bitmap_rect = gfx::Rect(gfx::Point(0, 0), size_); skia::PlatformCanvas* canvas = RenderProcess::current()->GetDrawingCanvas(¤t_paint_buf_, - damaged_rect); + bitmap_rect); if (!canvas) { NOTREACHED(); return; } - PaintRect(damaged_rect, canvas); + PaintRects(paint_rects, canvas); ViewHostMsg_PaintRect_Params params; - params.bitmap_rect = damaged_rect; + params.bitmap = current_paint_buf_->id(); + params.bitmap_rect = bitmap_rect; + params.paint_rects = paint_rects; params.view_size = size_; params.plugin_window_moves = plugin_window_moves_; params.flags = next_paint_flags_; - params.bitmap = current_paint_buf_->id(); delete canvas; @@ -473,12 +548,13 @@ void RenderWidget::DidInvalidateRect(WebWidget* webwidget, const WebRect& rect) { // We only want one pending DoDeferredPaint call at any time... - bool paint_pending = !paint_rect_.IsEmpty(); + bool paint_pending = !paint_rects_.empty(); // If this invalidate overlaps with a pending scroll, then we have to // downgrade to invalidating the scroll rect. + RectVectorHelper rect_vector(&paint_rects_); if (gfx::Rect(rect).Intersects(scroll_rect_)) { - paint_rect_ = paint_rect_.Union(scroll_rect_); + rect_vector.Add(scroll_rect_); scroll_rect_ = gfx::Rect(); } @@ -488,9 +564,9 @@ // Ignore invalidates that occur outside the bounds of the view // TODO(darin): maybe this should move into the paint code? // paint_rect_ = view_rect.Intersect(paint_rect_.Union(rect)); - paint_rect_ = paint_rect_.Union(view_rect.Intersect(rect)); + rect_vector.Add(view_rect.Intersect(rect)); - if (paint_rect_.IsEmpty() || paint_reply_pending() || paint_pending) + if (paint_rects_.empty() || paint_reply_pending() || paint_pending) return; // Perform painting asynchronously. This serves two purposes: @@ -510,7 +586,8 @@ dy = 0; } - bool intersects_with_painting = paint_rect_.Intersects(clip_rect); + bool intersects_with_painting = + RectVectorHelper(&paint_rects_).Intersects(clip_rect); // If we already have a pending scroll operation or if this scroll operation // intersects the existing paint region, then just failover to invalidating. Thanks! BYE MAD --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
