POSIX: bitmap data on the wire

From agl. Cleaned up version of issue 19046.

POSIX: bitmap data on the wire

On Windows, when drawing a given rect in the renderer, we allocate memory for the bitmap, render and send a shared memory handle across IPC. In the browser, we bitblit the shared memory to the backing store and draw it to the screen.

In the long term, on Linux, we want the backingstore to be shared with both X and the renderer. The renderer then draws directly to that store, sends an IPC to the browser and the browser sends a message to X to bitblit to the display. Since only cache a few backing stores we'll need messages from the browser to tell the renderer to attach and detatch from shared memory regions as they get created and evicted.

In the short term, however, to get something that works, we make a BitmapWireData typedef. This will be a shared memory region on Windows, as before, and on POSIX we'll be sending the bitmap data over the wire. Obviously this'll be pretty slow but it'll work sooner.

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

[Lei Zhang, 2009-01-30 02:04:16]

Primary reviewer: Brett.
Adding Darin per Evan's suggestion.

Succeeded on the try servers. Brett suggested trying to get
render_widget_host_unittests.cc to build on Linux, but it involved way too many
changes. (i.e. porting TestWebContents, TestingProfile, more scaffolding, fixing
forward declared enum LoadStates.)

[brettw, 2009-01-30 17:06:47]

http://codereview.chromium.org/19491/diff/1/6
File chrome/browser/browser.scons (right):

http://codereview.chromium.org/19491/diff/1/6#newcode841
Line 841: 'renderer_host/backing_store_posix.cc',
I'm a little unclear how this is supposed to work. Is the file list supposed to
list all the files and have the platforms remove them, or should it list the
common files and have the platforms add them? Looks like currently we do the
latter, so this change should probably do the same.

http://codereview.chromium.org/19491/diff/1/7
File chrome/browser/renderer_host/backing_store.h (right):

http://codereview.chromium.org/19491/diff/1/7#newcode73
Line 73: SkCanvas* canvas_;
It looks like this doesn't need to be a pointer and can just be a regular
member. Then you don't have to delete it. If you did need the pointer, it have
been a scoped_ptr.

http://codereview.chromium.org/19491/diff/1/9
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/19491/diff/1/9#newcode421
Line 421: DCHECK(params.bitmap);
I'd just remove this DCHECK. I don't think it buys us anything and with the
#ifdef just makes things more complicated.

http://codereview.chromium.org/19491/diff/1/10
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/19491/diff/1/10#newcode9
Line 9: #include <windows.h>
I think this can just be removed now. I checked and it's not required.

http://codereview.chromium.org/19491/diff/1/18
File chrome/common/bitmap_wire_data.h (right):

http://codereview.chromium.org/19491/diff/1/18#newcode15
Line 15: #if defined(OS_WIN)
Can you put blank lines around each of the preprocessor thingies here? I would
find it easier to read.

http://codereview.chromium.org/19491/diff/1/12
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/19491/diff/1/12#newcode1740
Line 1740: struct ParamTraits<scoped_refptr<SkBitmap> > {
I don't think this template is required. Probably it's left over from a previous
version of Adam's patch. You should be able to remove the extra skia includes at
the top as well.

http://codereview.chromium.org/19491/diff/1/16
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/19491/diff/1/16#newcode20
Line 20: #include <windows.h>
It's not clear what this is required for. Can you compile it without this
addition?

http://codereview.chromium.org/19491/diff/1/16#newcode430
Line 430: uint8_t* bitmap = (uint8_t*) malloc(GetPaintBufSize(damaged_rect));
I don't think you have to malloc the data yourself. You should just be able to
use the 3-argument constructor to PlatformCanvas (same below).

If we have to keep this line, it should be static_cast.

http://codereview.chromium.org/19491/diff/1/16#newcode523
Line 523: uint8_t* bitmap = (uint8_t*) malloc(GetPaintBufSize(damaged_rect));
c++ style casts here too.

http://codereview.chromium.org/19491/diff/1/5
File skia/ext/platform_canvas_mac.h (right):

http://codereview.chromium.org/19491/diff/1/5#newcode63
Line 63: static size_t StrideForWidth(unsigned width);
If you were able to remove the manual malloc in render_widget, you should be
able to remove this (and in the linux one, too) I think.

[Lei Zhang, 2009-01-31 01:42:06]

Also added backing_store_posix.cc to chrome/chrome.xcodeproj/project.pbxproj.

http://codereview.chromium.org/19491/diff/1/6
File chrome/browser/browser.scons (right):

http://codereview.chromium.org/19491/diff/1/6#newcode841
Line 841: 'renderer_host/backing_store_posix.cc',
On 2009/01/30 17:06:48, brettw wrote:
> I'm a little unclear how this is supposed to work. Is the file list supposed
to
> list all the files and have the platforms remove them, or should it list the
> common files and have the platforms add them? Looks like currently we do the
> latter, so this change should probably do the same.

From my understanding, we start out with a list for building on Windows - common
and Windows only files.

Then for non-Windows platforms, we remove the common files that have not been
ported, and we remove Windows-specific files. We should also add non-Windows
(i.e. *_posix*) here.

Then for each platform, we can do some more customizations. Please look at the
next revision and see if it's better.

http://codereview.chromium.org/19491/diff/1/7
File chrome/browser/renderer_host/backing_store.h (right):

http://codereview.chromium.org/19491/diff/1/7#newcode73
Line 73: SkCanvas* canvas_;
On 2009/01/30 17:06:48, brettw wrote:
> It looks like this doesn't need to be a pointer and can just be a regular
> member. Then you don't have to delete it. If you did need the pointer, it have
> been a scoped_ptr.

It's actually pointing to a PlatformCanvas. I made it a scoped_ptr.

http://codereview.chromium.org/19491/diff/1/9
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/19491/diff/1/9#newcode421
Line 421: DCHECK(params.bitmap);
On 2009/01/30 17:06:48, brettw wrote:
> I'd just remove this DCHECK. I don't think it buys us anything and with the
> #ifdef just makes things more complicated.

Ok.

http://codereview.chromium.org/19491/diff/1/10
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/19491/diff/1/10#newcode9
Line 9: #include <windows.h>
On 2009/01/30 17:06:48, brettw wrote:
> I think this can just be removed now. I checked and it's not required.

Done.

http://codereview.chromium.org/19491/diff/1/18
File chrome/common/bitmap_wire_data.h (right):

http://codereview.chromium.org/19491/diff/1/18#newcode15
Line 15: #if defined(OS_WIN)
On 2009/01/30 17:06:48, brettw wrote:
> Can you put blank lines around each of the preprocessor thingies here? I would
> find it easier to read.

Spaced it out and commented the #endif.

http://codereview.chromium.org/19491/diff/1/12
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/19491/diff/1/12#newcode1740
Line 1740: struct ParamTraits<scoped_refptr<SkBitmap> > {
On 2009/01/30 17:06:48, brettw wrote:
> I don't think this template is required. Probably it's left over from a
previous
> version of Adam's patch. You should be able to remove the extra skia includes
at
> the top as well.

Done. Though the extra skia includes are needed in render_widget.cc, so I moved
them there.

http://codereview.chromium.org/19491/diff/1/16
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/19491/diff/1/16#newcode20
Line 20: #include <windows.h>
On 2009/01/30 17:06:48, brettw wrote:
> It's not clear what this is required for. Can you compile it without this
> addition?

Agreed, I removed it.

http://codereview.chromium.org/19491/diff/1/16#newcode430
Line 430: uint8_t* bitmap = (uint8_t*) malloc(GetPaintBufSize(damaged_rect));
On 2009/01/30 17:06:48, brettw wrote:
> I don't think you have to malloc the data yourself. You should just be able to
> use the 3-argument constructor to PlatformCanvas (same below).
> 
> If we have to keep this line, it should be static_cast.

I made it static_cast. Don't we need bitmap on line 449? If we use the
3-argument constructor, how do we get the bitmap? I didn't see an obvious way,
but maybe I missed something.

[brettw, 2009-02-02 19:14:59]

http://codereview.chromium.org/19491/diff/224/229
File chrome/browser/renderer_host/backing_store.h (right):

http://codereview.chromium.org/19491/diff/224/229#newcode73
Line 73: scoped_ptr<SkCanvas> canvas_;
Can't you make this just a PlatformCanvas instead of a pointer?

http://codereview.chromium.org/19491/diff/224/226
File chrome/common/bitmap_wire_data.h (right):

http://codereview.chromium.org/19491/diff/224/226#newcode9
Line 9: #include "base/ref_counted.h"
Is this needed?

http://codereview.chromium.org/19491/diff/224/226#newcode11
Line 11: #include "skia/include/SkBitmap.h"
Can we just forward declare SkBitmap here?

http://codereview.chromium.org/19491/diff/224/236
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/19491/diff/224/236#newcode385
Line 385: static void SkBitmapSetup(SkBitmap* bitmap, const gfx::Rect& rect,
Can you rename this to something more descriptive like LoadDataIntoBitmap or
something?

http://codereview.chromium.org/19491/diff/224/236#newcode525
Line 525: uint8_t* bitmap = static_cast<uint8_t*>(
It seems like this should be unnecessary and just not pass data to the canvas
constructor.

[Evan Stade, 2009-02-02 19:25:19]

http://codereview.chromium.org/19491/diff/224/236
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/19491/diff/224/236#newcode383
Line 383: // Setup the given SkBitmap to take ownership of a 32-bit bitmap,
@data,
|data| rather than @data (same for others below)

(I know agl recently used @ to denote variable name, but || is more standard
across the codebase)

http://codereview.chromium.org/19491/diff/224/236#newcode390
Line 390: // @pixref takes ownership of @pixels
@pixels: no such variable (did you mean |data|?)

http://codereview.chromium.org/19491/diff/224/236#newcode396
Line 396: #endif
comment endif

(here and elsewhere)

http://codereview.chromium.org/19491/diff/224/236#newcode428
Line 428: #else
#elif defined(OS_POSIX)

(here and below)

http://codereview.chromium.org/19491/diff/224/236#newcode432
Line 432: malloc(GetPaintBufSize(damaged_rect)));
same deal here as below in DoDeferredScroll()

http://codereview.chromium.org/19491/diff/224/236#newcode525
Line 525: uint8_t* bitmap = static_cast<uint8_t*>(
On 2009/02/02 19:15:00, brettw wrote:
> It seems like this should be unnecessary and just not pass data to the canvas
> constructor.

yea it is unnecessary (and broken as it currently stands).

But we do need to access the memory region that backs the canvas (see
PaintRect() call lower down). Do you know how to do this?

[Evan Stade, 2009-02-02 19:28:56]

> But we do need to access the memory region that backs the canvas (see
> PaintRect() call lower down). Do you know how to do this?

sorry, I meant: see call to SkBitmapSetup(). It assumes |bitmap| will hold the
contents of the canvas.

[Lei Zhang, 2009-02-02 23:19:54]

Please take a look at patch set 4.

http://codereview.chromium.org/19491/diff/224/229
File chrome/browser/renderer_host/backing_store.h (right):

http://codereview.chromium.org/19491/diff/224/229#newcode73
Line 73: scoped_ptr<SkCanvas> canvas_;
On 2009/02/02 19:15:00, brettw wrote:
> Can't you make this just a PlatformCanvas instead of a pointer?

Right, done.

http://codereview.chromium.org/19491/diff/224/226
File chrome/common/bitmap_wire_data.h (right):

http://codereview.chromium.org/19491/diff/224/226#newcode9
Line 9: #include "base/ref_counted.h"
On 2009/02/02 19:15:00, brettw wrote:
> Is this needed?

Nope, removing.

http://codereview.chromium.org/19491/diff/224/226#newcode11
Line 11: #include "skia/include/SkBitmap.h"
On 2009/02/02 19:15:00, brettw wrote:
> Can we just forward declare SkBitmap here?

No, that would just make every file which #includes "bitmap_wire_data.h" have a
#include "skia/include/SkBitmap.h" line as well. I'll put #if defined(OS_POSIX)
around it though.

http://codereview.chromium.org/19491/diff/224/236
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/19491/diff/224/236#newcode383
Line 383: // Setup the given SkBitmap to take ownership of a 32-bit bitmap,
@data,
On 2009/02/02 19:25:19, estade wrote:
> |data| rather than @data (same for others below)
> 
> (I know agl recently used @ to denote variable name, but || is more standard
> across the codebase)

Done.

http://codereview.chromium.org/19491/diff/224/236#newcode385
Line 385: static void SkBitmapSetup(SkBitmap* bitmap, const gfx::Rect& rect,
On 2009/02/02 19:15:00, brettw wrote:
> Can you rename this to something more descriptive like LoadDataIntoBitmap or
> something?

How about LoadDataIntoSkBitmap?

http://codereview.chromium.org/19491/diff/224/236#newcode390
Line 390: // @pixref takes ownership of @pixels
On 2009/02/02 19:25:19, estade wrote:
> @pixels: no such variable (did you mean |data|?)

Yes.

http://codereview.chromium.org/19491/diff/224/236#newcode396
Line 396: #endif
On 2009/02/02 19:25:19, estade wrote:
> comment endif
> 
> (here and elsewhere)

Sure.

http://codereview.chromium.org/19491/diff/224/236#newcode428
Line 428: #else
On 2009/02/02 19:25:19, estade wrote:
> #elif defined(OS_POSIX)
> 
> (here and below)

Done.

http://codereview.chromium.org/19491/diff/224/236#newcode525
Line 525: uint8_t* bitmap = static_cast<uint8_t*>(
On 2009/02/02 19:25:19, estade wrote:
> On 2009/02/02 19:15:00, brettw wrote:
> > It seems like this should be unnecessary and just not pass data to the
canvas
> > constructor.
> 
> yea it is unnecessary (and broken as it currently stands).
> 
> But we do need to access the memory region that backs the canvas (see
> PaintRect() call lower down). Do you know how to do this?

Ah of course, canvas->getDevice()->accessBitmap(false)->getPixels(); !

[Evan Stade, 2009-02-02 23:57:27]

http://codereview.chromium.org/19491/diff/224/226
File chrome/common/bitmap_wire_data.h (right):

http://codereview.chromium.org/19491/diff/224/226#newcode11
Line 11: #include "skia/include/SkBitmap.h"
On 2009/02/02 23:19:54, Lei Zhang wrote:
> On 2009/02/02 19:15:00, brettw wrote:
> > Can we just forward declare SkBitmap here?
> 
> No, that would just make every file which #includes "bitmap_wire_data.h" have
a
> #include "skia/include/SkBitmap.h" line as well. I'll put #if
defined(OS_POSIX)
> around it though.

No, only .cc files that include this one will need to include SkBitmap.h. It is
preferred to keep the number of header files included by header files to a
minimum.

http://codereview.chromium.org/19491/diff/224/236
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/19491/diff/224/236#newcode385
Line 385: static void SkBitmapSetup(SkBitmap* bitmap, const gfx::Rect& rect,
Can't we just scrap this function and do

params.bitmap = canvas->getDevice()->accessBitmap(false)

[Lei Zhang, 2009-02-03 01:09:46]

Now at patch set 6.

http://codereview.chromium.org/19491/diff/224/226
File chrome/common/bitmap_wire_data.h (right):

http://codereview.chromium.org/19491/diff/224/226#newcode11
Line 11: #include "skia/include/SkBitmap.h"
On 2009/02/02 23:57:27, estade wrote:
> On 2009/02/02 23:19:54, Lei Zhang wrote:
> > On 2009/02/02 19:15:00, brettw wrote:
> > > Can we just forward declare SkBitmap here?
> > 
> > No, that would just make every file which #includes "bitmap_wire_data.h"
have
> a
> > #include "skia/include/SkBitmap.h" line as well. I'll put #if
> defined(OS_POSIX)
> > around it though.
> 
> No, only .cc files that include this one will need to include SkBitmap.h. It
is
> preferred to keep the number of header files included by header files to a
> minimum.

If I forward declare class SkBitmap, we just push the #include into
common/render_messages.h.

http://codereview.chromium.org/19491/diff/224/236
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/19491/diff/224/236#newcode385
Line 385: static void SkBitmapSetup(SkBitmap* bitmap, const gfx::Rect& rect,
On 2009/02/02 23:57:27, estade wrote:
> Can't we just scrap this function and do
> 
> params.bitmap = canvas->getDevice()->accessBitmap(false)
> 
> 

I scrapped it. Agl thinks that's ok, and we'll find out right away once we
exercise the code.

[Lei Zhang, 2009-02-03 01:14:38]

FYI Patch set 5 has #include "skia/ext/SkBitmap.h" where estade suggested it
should go.

[Evan Stade, 2009-02-03 01:22:43]

just some nits and LGTM (also be sure to take care of the 2 lint errors)

http://codereview.chromium.org/19491/diff/2413/2418
File chrome/browser/renderer_host/backing_store.h (right):

http://codereview.chromium.org/19491/diff/2413/2418#newcode75
Line 75: #endif   // defined(OS_WIN)
extra space before //

http://codereview.chromium.org/19491/diff/2413/2420
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/19491/diff/2413/2420#newcode22
Line 22: #endif   // defined(OS_WIN)
extra space before //

http://codereview.chromium.org/19491/diff/2413/2415
File chrome/common/bitmap_wire_data.h (right):

http://codereview.chromium.org/19491/diff/2413/2415#newcode16
Line 16: 
don't need the extra blank lines around these macros

http://codereview.chromium.org/19491/diff/2413/2425
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/19491/diff/2413/2425#newcode508
Line 508: true);
can fit this on line above

http://codereview.chromium.org/19491/diff/2413/2425#newcode525
Line 525: // POSIX currently passes the data itself. SkBuffer takes ownership of
update comment

http://codereview.chromium.org/19491/diff/2413/2426
File chrome/renderer/render_widget.h (right):

http://codereview.chromium.org/19491/diff/2413/2426#newcode108
Line 108: // Paints the given rectangular region of the WebWidget into paint_buf
(a
update comment

[brettw, 2009-02-03 01:30:36]

http://codereview.chromium.org/19491/diff/2413/2415
File chrome/common/bitmap_wire_data.h (right):

http://codereview.chromium.org/19491/diff/2413/2415#newcode16
Line 16: 
On 2009/02/03 01:22:43, estade wrote:
> don't need the extra blank lines around these macros

I asked for these, I think it's easier to read.

[brettw, 2009-02-03 01:33:46]

LGTM with this one change.

http://codereview.chromium.org/19491/diff/2413/2415
File chrome/common/bitmap_wire_data.h (right):

http://codereview.chromium.org/19491/diff/2413/2415#newcode9
Line 9: #include "skia/include/SkBitmap.h"
I thought you were going to forward declare this?

[Lei Zhang, 2009-02-03 01:47:53]

Addressed last round of comments in patch set 8.

http://codereview.chromium.org/19491/diff/2413/2415
File chrome/common/bitmap_wire_data.h (right):

http://codereview.chromium.org/19491/diff/2413/2415#newcode9
Line 9: #include "skia/include/SkBitmap.h"
On 2009/02/03 01:33:46, brettw wrote:
> I thought you were going to forward declare this?

Yep, reverted back to patch set 5.

http://codereview.chromium.org/19491/diff/2413/2425
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/19491/diff/2413/2425#newcode508
Line 508: true);
On 2009/02/03 01:22:43, estade wrote:
> can fit this on line above

Nope, goes over 80 chars per line by 1.

http://codereview.chromium.org/19491/diff/2413/2425#newcode525
Line 525: // POSIX currently passes the data itself. SkBuffer takes ownership of
On 2009/02/03 01:22:43, estade wrote:
> update comment

done.

http://codereview.chromium.org/19491/diff/2413/2426
File chrome/renderer/render_widget.h (right):

http://codereview.chromium.org/19491/diff/2413/2426#newcode108
Line 108: // Paints the given rectangular region of the WebWidget into paint_buf
(a
On 2009/02/03 01:22:43, estade wrote:
> update comment

Done.