PPAPI: Move PPB_ArrayBuffer out of Dev.

PPAPI: Move PPB_ArrayBuffer out of Dev.

BUG=103435
TEST=N/A


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

[dmichael (off chromium), 2012-01-12 20:59:55]

Are we ready to take this out of Dev? I'd like it to make 18. Two things I can
think of that could be worth changing before coming out of Dev:
- Make the C++ wrapper's "Map" not cache the pointer to the buffer. It might be
troublesome when combined with transferrables, so that a C++ ArrayBufferVar
could be "neutered", and it wouldn't be easy to detect. It would also fit with
our movement towards a "simple" C++ layer that directly translates the C
interface.
- Change ByteLength from:
uint32_t ByteLength(PP_Var);
to
PP_Bool ByteLength(PP_Var, uint32_t*)
Darin suggested this before. It's definitely the more common pattern in ppapi.

[darin (slow to review), 2012-01-12 21:14:02]

http://codereview.chromium.org/9107046/diff/2003/ppapi/api/ppb_var_array_buff...
File ppapi/api/ppb_var_array_buffer.idl (right):

http://codereview.chromium.org/9107046/diff/2003/ppapi/api/ppb_var_array_buff...
ppapi/api/ppb_var_array_buffer.idl:45: mem_t Map([in] PP_Var array);
Can you remind me of why there is no corresponding Unmap?  PPB_ImageData has
Unmap too.

[dmichael (off chromium), 2012-01-12 21:18:57]

On 2012/01/12 21:14:02, darin wrote:
>
http://codereview.chromium.org/9107046/diff/2003/ppapi/api/ppb_var_array_buff...
> File ppapi/api/ppb_var_array_buffer.idl (right):
> 
>
http://codereview.chromium.org/9107046/diff/2003/ppapi/api/ppb_var_array_buff...
> ppapi/api/ppb_var_array_buffer.idl:45: mem_t Map([in] PP_Var array);
> Can you remind me of why there is no corresponding Unmap?  PPB_ImageData has
> Unmap too.
I argued that caching the buffer pointer in the C++ wrapper's Map method,
combined with having an Unmap, was too dangerous. Without Unmap, you instead
release all references to the Var. If you had Unmap, some part of the code using
a VarArrayBuffer that already cached the pointer could stop working
mysteriously. This is similar to the problem we have if/when we do
transferrables.

Two ways around my objection to Unmap:
- stop caching the buffer pointer in the C++ wrapper
- keep a "map count" internally, so that the buffer is really only unmapped if
all users of the Var who have mapped it have unmapped.

[darin (slow to review), 2012-01-12 21:30:30]

On Thu, Jan 12, 2012 at 1:18 PM, <dmichael@chromium.org> wrote:

> On 2012/01/12 21:14:02, darin wrote:
>
> http://codereview.chromium.**org/9107046/diff/2003/ppapi/**
>
api/ppb_var_array_buffer.idl<http://codereview.chromium.org/9107046/diff/2003/ppapi/api/ppb_var_array_buffer.idl>
>
>> File ppapi/api/ppb_var_array_**buffer.idl (right):
>>
>
>
> http://codereview.chromium.**org/9107046/diff/2003/ppapi/**
>
api/ppb_var_array_buffer.idl#**newcode45<http://codereview.chromium.org/9107046/diff/2003/ppapi/api/ppb_var_array_buffer.idl#newcode45>
>
>> ppapi/api/ppb_var_array_**buffer.idl:45: mem_t Map([in] PP_Var array);
>> Can you remind me of why there is no corresponding Unmap?  PPB_ImageData
>> has
>> Unmap too.
>>
> I argued that caching the buffer pointer in the C++ wrapper's Map method,
> combined with having an Unmap, was too dangerous. Without Unmap, you
> instead
> release all references to the Var. If you had Unmap, some part of the code
> using
> a VarArrayBuffer that already cached the pointer could stop working
> mysteriously. This is similar to the problem we have if/when we do
> transferrables.
>
> Two ways around my objection to Unmap:
> - stop caching the buffer pointer in the C++ wrapper
> - keep a "map count" internally, so that the buffer is really only
> unmapped if
> all users of the Var who have mapped it have unmapped.
>
>
My preference is to make the C++ wrappers match the C interface closely,
and then provide utilities for convenience.  I don't think ImageBuffer
should be caching the mapped buffer in other words given how
PPB_ImageBuffer works.

I'd like for us to at least keep PPB_VarArrayBuffer consistent with
PPB_ImageBuffer.  It seems bad to use a different system.

Given the way pp::ImageData works, perhaps we should explore the reference
counting approach.  That could break existing apps.

Perhaps a third option is to hide the Unmap function in the C++ layer?
 That appears to be what ImageData does.

-Darin



>
http://codereview.chromium.**org/9107046/<http://codereview.chromium.org/9107...
>

[dmichael (off chromium), 2012-01-12 21:47:44]

On 2012/01/12 21:30:30, darin wrote:
> On Thu, Jan 12, 2012 at 1:18 PM, <mailto:dmichael@chromium.org> wrote:
> 
> > On 2012/01/12 21:14:02, darin wrote:
> >
> > http://codereview.chromium.**org/9107046/diff/2003/ppapi/**
> >
>
api/ppb_var_array_buffer.idl<http://codereview.chromium.org/9107046/diff/2003/ppapi/api/ppb_var_array_buffer.idl>
> >
> >> File ppapi/api/ppb_var_array_**buffer.idl (right):
> >>
> >
> >
> > http://codereview.chromium.**org/9107046/diff/2003/ppapi/**
> >
>
api/ppb_var_array_buffer.idl#**newcode45<http://codereview.chromium.org/9107046/diff/2003/ppapi/api/ppb_var_array_buffer.idl#newcode45>
> >
> >> ppapi/api/ppb_var_array_**buffer.idl:45: mem_t Map([in] PP_Var array);
> >> Can you remind me of why there is no corresponding Unmap?  PPB_ImageData
> >> has
> >> Unmap too.
> >>
> > I argued that caching the buffer pointer in the C++ wrapper's Map method,
> > combined with having an Unmap, was too dangerous. Without Unmap, you
> > instead
> > release all references to the Var. If you had Unmap, some part of the code
> > using
> > a VarArrayBuffer that already cached the pointer could stop working
> > mysteriously. This is similar to the problem we have if/when we do
> > transferrables.
> >
> > Two ways around my objection to Unmap:
> > - stop caching the buffer pointer in the C++ wrapper
> > - keep a "map count" internally, so that the buffer is really only
> > unmapped if
> > all users of the Var who have mapped it have unmapped.
> >
> >
> My preference is to make the C++ wrappers match the C interface closely,
> and then provide utilities for convenience.  I don't think ImageBuffer
> should be caching the mapped buffer in other words given how
> PPB_ImageBuffer works.
Agreed.
> 
> I'd like for us to at least keep PPB_VarArrayBuffer consistent with
> PPB_ImageBuffer.  It seems bad to use a different system.
> 
> Given the way pp::ImageData works, perhaps we should explore the reference
> counting approach.
By which you mean count Maps and Unmaps? For what it's worth, this is already
what PPB_Buffer_Dev does in the OOP proxy. Current behavior appears to be:
PPB_ImageData.Unmap, NaCl proxy: really does unmap
PPB_ImageData.Unmap, OOP proxy: does nothing
PPB_Buffer_Dev.Unmap, NaCl proxy: not implemented.
PPB_Buffer_Dev.Unmap, OOP proxy: counts Maps/Unmaps; really unmaps when count
reaches 0.
>  That could break existing apps.
It seems unlikely, though. If they're doing Map in 2 or more places now and
Unmap in 2 or more places, they might already be in danger. I guess it seems
equally likely it could "fix" existing apps :-)

Note, however, that adding a count won't help us if/when we implement
transferables. If a VarArrayBuffer is transfered, we have to neuter it,
regardless of whether any outstanding references exist which have "Mapped" it.
In that sense, VarArrayBuffer would be different, so it might not be crazy for
its C++ wrapper to be different (i.e., not cache the buffer pointer as ImageData
does, since it would be inherently more dangerous for VarArrayBuffer).
> 
> Perhaps a third option is to hide the Unmap function in the C++ layer?
>  That appears to be what ImageData does.
That is kind of a weak protection, though, since you could still Unmap it from
the C interface.

I'm leaning towards:
-Adding Unmap for consistency. We can make it not reference count, to be
consistent (or maybe better, make ImageData start counting).
-Eliminating the caching from pp::VarArrayBuffer::Map. I think it might come
back to bite us if/when we do transferrables. I can add good documentation
showing a good & bad example of how to write a loop. In this world, a bug means
bad performance rather than a latent crasher. Seems to me like a good tradeoff.
> 
> -Darin
> 
> 
> 
> >
>
http://codereview.chromium.**org/9107046/%3Chttp://codereview.chromium.org/91...>
> >

[dmichael (off chromium), 2012-01-18 20:52:38]

On 2012/01/12 21:47:44, dmichael wrote:
> On 2012/01/12 21:30:30, darin wrote:
> > On Thu, Jan 12, 2012 at 1:18 PM, <mailto:dmichael@chromium.org> wrote:
> > 
> > > On 2012/01/12 21:14:02, darin wrote:
> > >
> > > http://codereview.chromium.**org/9107046/diff/2003/ppapi/**
> > >
> >
>
api/ppb_var_array_buffer.idl<http://codereview.chromium.org/9107046/diff/2003/ppapi/api/ppb_var_array_buffer.idl>
> > >
> > >> File ppapi/api/ppb_var_array_**buffer.idl (right):
> > >>
> > >
> > >
> > > http://codereview.chromium.**org/9107046/diff/2003/ppapi/**
> > >
> >
>
api/ppb_var_array_buffer.idl#**newcode45<http://codereview.chromium.org/9107046/diff/2003/ppapi/api/ppb_var_array_buffer.idl#newcode45>
> > >
> > >> ppapi/api/ppb_var_array_**buffer.idl:45: mem_t Map([in] PP_Var array);
> > >> Can you remind me of why there is no corresponding Unmap?  PPB_ImageData
> > >> has
> > >> Unmap too.
> > >>
> > > I argued that caching the buffer pointer in the C++ wrapper's Map method,
> > > combined with having an Unmap, was too dangerous. Without Unmap, you
> > > instead
> > > release all references to the Var. If you had Unmap, some part of the code
> > > using
> > > a VarArrayBuffer that already cached the pointer could stop working
> > > mysteriously. This is similar to the problem we have if/when we do
> > > transferrables.
> > >
> > > Two ways around my objection to Unmap:
> > > - stop caching the buffer pointer in the C++ wrapper
> > > - keep a "map count" internally, so that the buffer is really only
> > > unmapped if
> > > all users of the Var who have mapped it have unmapped.
> > >
> > >
> > My preference is to make the C++ wrappers match the C interface closely,
> > and then provide utilities for convenience.  I don't think ImageBuffer
> > should be caching the mapped buffer in other words given how
> > PPB_ImageBuffer works.
> Agreed.
> > 
> > I'd like for us to at least keep PPB_VarArrayBuffer consistent with
> > PPB_ImageBuffer.  It seems bad to use a different system.
> > 
> > Given the way pp::ImageData works, perhaps we should explore the reference
> > counting approach.
> By which you mean count Maps and Unmaps? For what it's worth, this is already
> what PPB_Buffer_Dev does in the OOP proxy. Current behavior appears to be:
> PPB_ImageData.Unmap, NaCl proxy: really does unmap
> PPB_ImageData.Unmap, OOP proxy: does nothing
> PPB_Buffer_Dev.Unmap, NaCl proxy: not implemented.
> PPB_Buffer_Dev.Unmap, OOP proxy: counts Maps/Unmaps; really unmaps when count
> reaches 0.
> >  That could break existing apps.
> It seems unlikely, though. If they're doing Map in 2 or more places now and
> Unmap in 2 or more places, they might already be in danger. I guess it seems
> equally likely it could "fix" existing apps :-)
> 
> Note, however, that adding a count won't help us if/when we implement
> transferables. If a VarArrayBuffer is transfered, we have to neuter it,
> regardless of whether any outstanding references exist which have "Mapped" it.
> In that sense, VarArrayBuffer would be different, so it might not be crazy for
> its C++ wrapper to be different (i.e., not cache the buffer pointer as
ImageData
> does, since it would be inherently more dangerous for VarArrayBuffer).
> > 
> > Perhaps a third option is to hide the Unmap function in the C++ layer?
> >  That appears to be what ImageData does.
> That is kind of a weak protection, though, since you could still Unmap it from
> the C interface.
> 
Ping? Any thoughts about the following proposal:

> I'm leaning towards:
> -Adding Unmap for consistency. We can make it not reference count, to be
> consistent (or maybe better, make ImageData start counting).
> -Eliminating the caching from pp::VarArrayBuffer::Map. I think it might come
> back to bite us if/when we do transferrables. I can add good documentation
> showing a good & bad example of how to write a loop. In this world, a bug
means
> bad performance rather than a latent crasher. Seems to me like a good
tradeoff.
> > 
> > -Darin
> > 
> > 
> > 
> > >
> >
>
http://codereview.chromium.**org/9107046/%253Chttp://codereview.chromium.org/...>
> > >

[darin (slow to review), 2012-01-18 22:00:57]

On Wed, Jan 18, 2012 at 12:52 PM, <dmichael@chromium.org> wrote:

> On 2012/01/12 21:47:44, dmichael wrote:
>
>> On 2012/01/12 21:30:30, darin wrote:
>> > On Thu, Jan 12, 2012 at 1:18 PM, <mailto:dmichael@chromium.org> wrote:
>> >
>> > > On 2012/01/12 21:14:02, darin wrote:
>> > >
>> > > http://codereview.chromium.****org/9107046/diff/2003/ppapi/**
>> > >
>> >
>>
>
> api/ppb_var_array_buffer.idl<h**ttp://codereview.chromium.org/**
>
9107046/diff/2003/ppapi/api/**ppb_var_array_buffer.idl<http://codereview.chromium.org/9107046/diff/2003/ppapi/api/ppb_var_array_buffer.idl>
> >
>
>> > >
>> > >> File ppapi/api/ppb_var_array_****buffer.idl (right):
>> > >>
>> > >
>> > >
>> > > http://codereview.chromium.****org/9107046/diff/2003/ppapi/**
>> > >
>> >
>>
>
> api/ppb_var_array_buffer.idl#****newcode45<http://codereview.**
> chromium.org/9107046/diff/**2003/ppapi/api/ppb_var_array_**
>
buffer.idl#newcode45<http://codereview.chromium.org/9107046/diff/2003/ppapi/api/ppb_var_array_buffer.idl#newcode45>
> >
>
>> > >
>> > >> ppapi/api/ppb_var_array_****buffer.idl:45: mem_t Map([in] PP_Var
>> array);
>> > >> Can you remind me of why there is no corresponding Unmap?
>>  PPB_ImageData
>> > >> has
>> > >> Unmap too.
>> > >>
>> > > I argued that caching the buffer pointer in the C++ wrapper's Map
>> method,
>> > > combined with having an Unmap, was too dangerous. Without Unmap, you
>> > > instead
>> > > release all references to the Var. If you had Unmap, some part of the
>> code
>> > > using
>> > > a VarArrayBuffer that already cached the pointer could stop working
>> > > mysteriously. This is similar to the problem we have if/when we do
>> > > transferrables.
>> > >
>> > > Two ways around my objection to Unmap:
>> > > - stop caching the buffer pointer in the C++ wrapper
>> > > - keep a "map count" internally, so that the buffer is really only
>> > > unmapped if
>> > > all users of the Var who have mapped it have unmapped.
>> > >
>> > >
>> > My preference is to make the C++ wrappers match the C interface closely,
>> > and then provide utilities for convenience.  I don't think ImageBuffer
>> > should be caching the mapped buffer in other words given how
>> > PPB_ImageBuffer works.
>> Agreed.
>> >
>> > I'd like for us to at least keep PPB_VarArrayBuffer consistent with
>> > PPB_ImageBuffer.  It seems bad to use a different system.
>> >
>> > Given the way pp::ImageData works, perhaps we should explore the
>> reference
>> > counting approach.
>> By which you mean count Maps and Unmaps? For what it's worth, this is
>> already
>> what PPB_Buffer_Dev does in the OOP proxy. Current behavior appears to be:
>> PPB_ImageData.Unmap, NaCl proxy: really does unmap
>> PPB_ImageData.Unmap, OOP proxy: does nothing
>> PPB_Buffer_Dev.Unmap, NaCl proxy: not implemented.
>> PPB_Buffer_Dev.Unmap, OOP proxy: counts Maps/Unmaps; really unmaps when
>> count
>> reaches 0.
>> >  That could break existing apps.
>> It seems unlikely, though. If they're doing Map in 2 or more places now
>> and
>> Unmap in 2 or more places, they might already be in danger. I guess it
>> seems
>> equally likely it could "fix" existing apps :-)
>>
>
>  Note, however, that adding a count won't help us if/when we implement
>> transferables. If a VarArrayBuffer is transfered, we have to neuter it,
>> regardless of whether any outstanding references exist which have
>> "Mapped" it.
>> In that sense, VarArrayBuffer would be different, so it might not be
>> crazy for
>> its C++ wrapper to be different (i.e., not cache the buffer pointer as
>>
> ImageData
>
>> does, since it would be inherently more dangerous for VarArrayBuffer).
>> >
>> > Perhaps a third option is to hide the Unmap function in the C++ layer?
>> >  That appears to be what ImageData does.
>> That is kind of a weak protection, though, since you could still Unmap it
>> from
>> the C interface.
>>
>
> Ping? Any thoughts about the following proposal:
>
>
Sorry for the delay in responding!!  I'm OK with that proposal.
-Darin



>
>  I'm leaning towards:
>> -Adding Unmap for consistency. We can make it not reference count, to be
>> consistent (or maybe better, make ImageData start counting).
>> -Eliminating the caching from pp::VarArrayBuffer::Map. I think it might
>> come
>> back to bite us if/when we do transferrables. I can add good documentation
>> showing a good & bad example of how to write a loop. In this world, a bug
>>
> means
>
>> bad performance rather than a latent crasher. Seems to me like a good
>>
> tradeoff.
>
>> >
>> > -Darin
>> >
>> >
>> >
>> > >
>> >
>>
>
> http://codereview.chromium.****org/9107046/%253Chttp://codere**
> view.chromium.org/9107046/ <http://codereview.chromium.org/9107046/>>
>
>> > >
>>
>
>
>
>
http://codereview.chromium.**org/9107046/<http://codereview.chromium.org/9107...
>

[dmichael (off chromium), 2012-01-19 19:20:44]

PTAL:
-Added Unmap, though it currently does nothing.
-Removed caching from the C++ wrapper.
-Decided against counting Map/Unmap. Without caching in the C++ wrapper, the
risk I was worried about doesn't really exist. This is simpler and more
consistent with ImageData.

[dmichael (off chromium), 2012-01-26 22:37:05]

bbudge primary, darin & brettw FYI

[bbudge, 2012-01-27 00:49:05]

lgtm

[commit-bot: I haz the power, 2012-01-27 01:27:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/9107046/14024

[commit-bot: I haz the power, 2012-01-27 04:55:58]

Change committed as 119374