Rename shared memory methods for consistency.

Rename shared memory method parameters for consistency.

Brings internal consistency to the shared memory support
methods since it doesn't look like we'll be able to get
rid of them anytime soon.

BUG=none
TEST=none

[DaleCurtis, 2012-08-25 00:06:44]

crogers made the point on
http://codereview.chromium.org/10832285/diff/23001/content/browser/renderer_h...

that packet is a bad name. When looking into his comment I noticed the method
names are a bit misleading here regardless of whether we switch away from packet
or not.

I realize this is probably a bikeshed, but while I'm cleaning everything else
up, why not? What does everyone else think? Personally, I'm fine with the packet
name since the data is involved in transfers even if they aren't of the network
or codec type.

[scherkus (not reviewing), 2012-08-27 20:52:38]

Wait I'm a bit confused... "packet" is a bad name but this CL has us switching
to "packet"?

Is packet_size here always equal to the underlying size of the shared memory?

[DaleCurtis, 2012-08-27 21:08:41]

On 2012/08/27 20:52:38, scherkus wrote:
> Wait I'm a bit confused... "packet" is a bad name but this CL has us switching
> to "packet"?
> 
> Is packet_size here always equal to the underlying size of the shared memory?

Your comment is the reason for the rename, these functions are actually looking
for the size of the "shared memory - sizeof(Atomic32)", aka the value returned
by PacketSizeInBytes(). This can be seen in each method by the "char* ptr =
static_cast<char*>(shared_memory->memory()) + packet_size;" Where ptr is
supposed to point to the atomic32 at the end of shared memory.

The CL upload just makes the naming internally consistent. If you want a better
name, please suggest :)

[scherkus (not reviewing), 2012-08-28 00:01:41]

On 2012/08/27 21:08:41, DaleCurtis wrote:
> On 2012/08/27 20:52:38, scherkus wrote:
> > Wait I'm a bit confused... "packet" is a bad name but this CL has us
switching
> > to "packet"?
> > 
> > Is packet_size here always equal to the underlying size of the shared
memory?
> 
> Your comment is the reason for the rename, these functions are actually
looking
> for the size of the "shared memory - sizeof(Atomic32)", aka the value returned
> by PacketSizeInBytes(). This can be seen in each method by the "char* ptr =
> static_cast<char*>(shared_memory->memory()) + packet_size;" Where ptr is
> supposed to point to the atomic32 at the end of shared memory.
> 
> The CL upload just makes the naming internally consistent. If you want a
better
> name, please suggest :)

If we expect this code to be hanging around a bit longer do you think we should
pull the trigger and create a wrapper class that makes it easier to use?

Some of the code we have in audio_output_device.cc is frightening.

[Chris Rogers, 2012-08-28 00:39:23]

On 2012/08/28 00:01:41, scherkus wrote:
> On 2012/08/27 21:08:41, DaleCurtis wrote:
> > On 2012/08/27 20:52:38, scherkus wrote:
> > > Wait I'm a bit confused... "packet" is a bad name but this CL has us
> switching
> > > to "packet"?
> > > 
> > > Is packet_size here always equal to the underlying size of the shared
> memory?
> > 
> > Your comment is the reason for the rename, these functions are actually
> looking
> > for the size of the "shared memory - sizeof(Atomic32)", aka the value
returned
> > by PacketSizeInBytes(). This can be seen in each method by the "char* ptr =
> > static_cast<char*>(shared_memory->memory()) + packet_size;" Where ptr is
> > supposed to point to the atomic32 at the end of shared memory.
> > 
> > The CL upload just makes the naming internally consistent. If you want a
> better
> > name, please suggest :)
> 
> If we expect this code to be hanging around a bit longer do you think we
should
> pull the trigger and create a wrapper class that makes it easier to use?
> 
> Some of the code we have in audio_output_device.cc is frightening.

Yeah, I agree and am not much of a fan of the "magic bytes".  I've spoken with
Nicholas about it and he doesn't like it either.  I'm hoping we can get rid of
that stuff in the not-to-distant future, after <audio> switches to low-latency
path and we're only using waveOut on XP.

[DaleCurtis, 2012-08-28 00:57:30]

On 2012/08/28 00:39:23, Chris Rogers wrote:
> On 2012/08/28 00:01:41, scherkus wrote:
> > On 2012/08/27 21:08:41, DaleCurtis wrote:
> > > On 2012/08/27 20:52:38, scherkus wrote:
> > > > Wait I'm a bit confused... "packet" is a bad name but this CL has us
> > switching
> > > > to "packet"?
> > > > 
> > > > Is packet_size here always equal to the underlying size of the shared
> > memory?
> > > 
> > > Your comment is the reason for the rename, these functions are actually
> > looking
> > > for the size of the "shared memory - sizeof(Atomic32)", aka the value
> returned
> > > by PacketSizeInBytes(). This can be seen in each method by the "char* ptr
=
> > > static_cast<char*>(shared_memory->memory()) + packet_size;" Where ptr is
> > > supposed to point to the atomic32 at the end of shared memory.
> > > 
> > > The CL upload just makes the naming internally consistent. If you want a
> > better
> > > name, please suggest :)
> > 
> > If we expect this code to be hanging around a bit longer do you think we
> should
> > pull the trigger and create a wrapper class that makes it easier to use?
> > 
> > Some of the code we have in audio_output_device.cc is frightening.
> 
> Yeah, I agree and am not much of a fan of the "magic bytes".  I've spoken with
> Nicholas about it and he doesn't like it either.  I'm hoping we can get rid of
> that stuff in the not-to-distant future, after <audio> switches to low-latency
> path and we're only using waveOut on XP.

I'd love to get rid of these as much as anyone else, but I don't think we've
found a solution that allows us to do that yet.  WebRTC requires accurate timing
information and so does <audio> to an extent.  Even with low latency, a fixed
delay is probably insufficient since buffers don't seem to stabilize (at least
on Linux).

I don't know what kind of wrapper class we could have here aside from moving
this complexity into AudioBus, which I haven't fully thought through.

[scherkus (not reviewing), 2012-08-28 01:35:54]

On 2012/08/28 00:57:30, DaleCurtis wrote:
> On 2012/08/28 00:39:23, Chris Rogers wrote:
> > On 2012/08/28 00:01:41, scherkus wrote:
> > > On 2012/08/27 21:08:41, DaleCurtis wrote:
> > > > On 2012/08/27 20:52:38, scherkus wrote:
> > > > > Wait I'm a bit confused... "packet" is a bad name but this CL has us
> > > switching
> > > > > to "packet"?
> > > > > 
> > > > > Is packet_size here always equal to the underlying size of the shared
> > > memory?
> > > > 
> > > > Your comment is the reason for the rename, these functions are actually
> > > looking
> > > > for the size of the "shared memory - sizeof(Atomic32)", aka the value
> > returned
> > > > by PacketSizeInBytes(). This can be seen in each method by the "char*
ptr
> =
> > > > static_cast<char*>(shared_memory->memory()) + packet_size;" Where ptr is
> > > > supposed to point to the atomic32 at the end of shared memory.
> > > > 
> > > > The CL upload just makes the naming internally consistent. If you want a
> > > better
> > > > name, please suggest :)
> > > 
> > > If we expect this code to be hanging around a bit longer do you think we
> > should
> > > pull the trigger and create a wrapper class that makes it easier to use?
> > > 
> > > Some of the code we have in audio_output_device.cc is frightening.
> > 
> > Yeah, I agree and am not much of a fan of the "magic bytes".  I've spoken
with
> > Nicholas about it and he doesn't like it either.  I'm hoping we can get rid
of
> > that stuff in the not-to-distant future, after <audio> switches to
low-latency
> > path and we're only using waveOut on XP.
> 
> I'd love to get rid of these as much as anyone else, but I don't think we've
> found a solution that allows us to do that yet.  WebRTC requires accurate
timing
> information and so does <audio> to an extent.  Even with low latency, a fixed
> delay is probably insufficient since buffers don't seem to stabilize (at least
> on Linux).
> 
> I don't know what kind of wrapper class we could have here aside from moving
> this complexity into AudioBus, which I haven't fully thought through.

I was thinking of something that hides the memory layout + total size meaning
clients only have to work with a SharedMemory* and its audio data/packet size
and not worry about which size to pass into which functions.

class AudioSharedMemory {
 public:
  explicit AudioSharedMemory(handle);

  // Used by AudioRendererHost
  bool CreateAndMap(audio_data_size);

  // Used by AudioOutputDevice in render process
  bool Map(audio_data_size);

  // Gets our data pointer.
  void* data();

  // Replaces Set/GetActualDataSizeInBytes(). These names could
  // also be improved.
  int actual_data_size();
  void set_actual_data_size();

  // Do we still need these!?
  void set_unknown_data_size();
  bool is_data_size_unknown();
};

Again, haven't looked at this in detail but if we'll be keeping this around for
a bit longer we might as well make it less crazy.

WDYT?

[DaleCurtis, 2012-08-28 01:49:25]

On 2012/08/28 01:35:54, scherkus wrote:
> On 2012/08/28 00:57:30, DaleCurtis wrote:
> > On 2012/08/28 00:39:23, Chris Rogers wrote:
> > > On 2012/08/28 00:01:41, scherkus wrote:
> > > > On 2012/08/27 21:08:41, DaleCurtis wrote:
> > > > > On 2012/08/27 20:52:38, scherkus wrote:
> > > > > > Wait I'm a bit confused... "packet" is a bad name but this CL has us
> > > > switching
> > > > > > to "packet"?
> > > > > > 
> > > > > > Is packet_size here always equal to the underlying size of the
shared
> > > > memory?
> > > > > 
> > > > > Your comment is the reason for the rename, these functions are
actually
> > > > looking
> > > > > for the size of the "shared memory - sizeof(Atomic32)", aka the value
> > > returned
> > > > > by PacketSizeInBytes(). This can be seen in each method by the "char*
> ptr
> > =
> > > > > static_cast<char*>(shared_memory->memory()) + packet_size;" Where ptr
is
> > > > > supposed to point to the atomic32 at the end of shared memory.
> > > > > 
> > > > > The CL upload just makes the naming internally consistent. If you want
a
> > > > better
> > > > > name, please suggest :)
> > > > 
> > > > If we expect this code to be hanging around a bit longer do you think we
> > > should
> > > > pull the trigger and create a wrapper class that makes it easier to use?
> > > > 
> > > > Some of the code we have in audio_output_device.cc is frightening.
> > > 
> > > Yeah, I agree and am not much of a fan of the "magic bytes".  I've spoken
> with
> > > Nicholas about it and he doesn't like it either.  I'm hoping we can get
rid
> of
> > > that stuff in the not-to-distant future, after <audio> switches to
> low-latency
> > > path and we're only using waveOut on XP.
> > 
> > I'd love to get rid of these as much as anyone else, but I don't think we've
> > found a solution that allows us to do that yet.  WebRTC requires accurate
> timing
> > information and so does <audio> to an extent.  Even with low latency, a
fixed
> > delay is probably insufficient since buffers don't seem to stabilize (at
least
> > on Linux).
> > 
> > I don't know what kind of wrapper class we could have here aside from moving
> > this complexity into AudioBus, which I haven't fully thought through.
> 
> I was thinking of something that hides the memory layout + total size meaning
> clients only have to work with a SharedMemory* and its audio data/packet size
> and not worry about which size to pass into which functions.
> 
> class AudioSharedMemory {
>  public:
>   explicit AudioSharedMemory(handle);
> 
>   // Used by AudioRendererHost
>   bool CreateAndMap(audio_data_size);
> 
>   // Used by AudioOutputDevice in render process
>   bool Map(audio_data_size);
> 
>   // Gets our data pointer.
>   void* data();
> 
>   // Replaces Set/GetActualDataSizeInBytes(). These names could
>   // also be improved.
>   int actual_data_size();
>   void set_actual_data_size();
> 
>   // Do we still need these!?
>   void set_unknown_data_size();
>   bool is_data_size_unknown();
> };
> 
> Again, haven't looked at this in detail but if we'll be keeping this around
for
> a bit longer we might as well make it less crazy.
> 
> WDYT?

I like it, but the old NaCl doesn't use SharedMemory or SharedMemoryHandle, just
an integer handle casted to a NaClHandle; which is why there's the
SetActualDataSizeInBytes(void *) version (see browser_ppb_audio_rpc_server.cc). 
They have a custom Map() process too.  Fixing the PPAPI side might be easier
since they at least have SharedMemory and other constructs we're familiar with. 
I don't think either fix would be simple though.

(Also all those methods are still needed, the unknown data size ones are used
inside audio_sync_reader.)

[scherkus (not reviewing), 2012-08-28 03:26:20]

On 2012/08/28 01:49:25, DaleCurtis wrote:
> On 2012/08/28 01:35:54, scherkus wrote:
> > On 2012/08/28 00:57:30, DaleCurtis wrote:
> > > On 2012/08/28 00:39:23, Chris Rogers wrote:
> > > > On 2012/08/28 00:01:41, scherkus wrote:
> > > > > On 2012/08/27 21:08:41, DaleCurtis wrote:
> > > > > > On 2012/08/27 20:52:38, scherkus wrote:
> > > > > > > Wait I'm a bit confused... "packet" is a bad name but this CL has
us
> > > > > switching
> > > > > > > to "packet"?
> > > > > > > 
> > > > > > > Is packet_size here always equal to the underlying size of the
> shared
> > > > > memory?
> > > > > > 
> > > > > > Your comment is the reason for the rename, these functions are
> actually
> > > > > looking
> > > > > > for the size of the "shared memory - sizeof(Atomic32)", aka the
value
> > > > returned
> > > > > > by PacketSizeInBytes(). This can be seen in each method by the
"char*
> > ptr
> > > =
> > > > > > static_cast<char*>(shared_memory->memory()) + packet_size;" Where
ptr
> is
> > > > > > supposed to point to the atomic32 at the end of shared memory.
> > > > > > 
> > > > > > The CL upload just makes the naming internally consistent. If you
want
> a
> > > > > better
> > > > > > name, please suggest :)
> > > > > 
> > > > > If we expect this code to be hanging around a bit longer do you think
we
> > > > should
> > > > > pull the trigger and create a wrapper class that makes it easier to
use?
> > > > > 
> > > > > Some of the code we have in audio_output_device.cc is frightening.
> > > > 
> > > > Yeah, I agree and am not much of a fan of the "magic bytes".  I've
spoken
> > with
> > > > Nicholas about it and he doesn't like it either.  I'm hoping we can get
> rid
> > of
> > > > that stuff in the not-to-distant future, after <audio> switches to
> > low-latency
> > > > path and we're only using waveOut on XP.
> > > 
> > > I'd love to get rid of these as much as anyone else, but I don't think
we've
> > > found a solution that allows us to do that yet.  WebRTC requires accurate
> > timing
> > > information and so does <audio> to an extent.  Even with low latency, a
> fixed
> > > delay is probably insufficient since buffers don't seem to stabilize (at
> least
> > > on Linux).
> > > 
> > > I don't know what kind of wrapper class we could have here aside from
moving
> > > this complexity into AudioBus, which I haven't fully thought through.
> > 
> > I was thinking of something that hides the memory layout + total size
meaning
> > clients only have to work with a SharedMemory* and its audio data/packet
size
> > and not worry about which size to pass into which functions.
> > 
> > class AudioSharedMemory {
> >  public:
> >   explicit AudioSharedMemory(handle);
> > 
> >   // Used by AudioRendererHost
> >   bool CreateAndMap(audio_data_size);
> > 
> >   // Used by AudioOutputDevice in render process
> >   bool Map(audio_data_size);
> > 
> >   // Gets our data pointer.
> >   void* data();
> > 
> >   // Replaces Set/GetActualDataSizeInBytes(). These names could
> >   // also be improved.
> >   int actual_data_size();
> >   void set_actual_data_size();
> > 
> >   // Do we still need these!?
> >   void set_unknown_data_size();
> >   bool is_data_size_unknown();
> > };
> > 
> > Again, haven't looked at this in detail but if we'll be keeping this around
> for
> > a bit longer we might as well make it less crazy.
> > 
> > WDYT?
> 
> I like it, but the old NaCl doesn't use SharedMemory or SharedMemoryHandle,
just
> an integer handle casted to a NaClHandle; which is why there's the
> SetActualDataSizeInBytes(void *) version (see
browser_ppb_audio_rpc_server.cc). 
> They have a custom Map() process too.  Fixing the PPAPI side might be easier
> since they at least have SharedMemory and other constructs we're familiar
with. 
> I don't think either fix would be simple though.
> 
> (Also all those methods are still needed, the unknown data size ones are used
> inside audio_sync_reader.)

OK -- in which case LGTM

[DaleCurtis, 2012-08-29 22:50:06]

On 2012/08/28 03:26:20, scherkus wrote:
> On 2012/08/28 01:49:25, DaleCurtis wrote:
> > On 2012/08/28 01:35:54, scherkus wrote:
> > > On 2012/08/28 00:57:30, DaleCurtis wrote:
> > > > On 2012/08/28 00:39:23, Chris Rogers wrote:
> > > > > On 2012/08/28 00:01:41, scherkus wrote:
> > > > > > On 2012/08/27 21:08:41, DaleCurtis wrote:
> > > > > > > On 2012/08/27 20:52:38, scherkus wrote:
> > > > > > > > Wait I'm a bit confused... "packet" is a bad name but this CL
has
> us
> > > > > > switching
> > > > > > > > to "packet"?
> > > > > > > > 
> > > > > > > > Is packet_size here always equal to the underlying size of the
> > shared
> > > > > > memory?
> > > > > > > 
> > > > > > > Your comment is the reason for the rename, these functions are
> > actually
> > > > > > looking
> > > > > > > for the size of the "shared memory - sizeof(Atomic32)", aka the
> value
> > > > > returned
> > > > > > > by PacketSizeInBytes(). This can be seen in each method by the
> "char*
> > > ptr
> > > > =
> > > > > > > static_cast<char*>(shared_memory->memory()) + packet_size;" Where
> ptr
> > is
> > > > > > > supposed to point to the atomic32 at the end of shared memory.
> > > > > > > 
> > > > > > > The CL upload just makes the naming internally consistent. If you
> want
> > a
> > > > > > better
> > > > > > > name, please suggest :)
> > > > > > 
> > > > > > If we expect this code to be hanging around a bit longer do you
think
> we
> > > > > should
> > > > > > pull the trigger and create a wrapper class that makes it easier to
> use?
> > > > > > 
> > > > > > Some of the code we have in audio_output_device.cc is frightening.
> > > > > 
> > > > > Yeah, I agree and am not much of a fan of the "magic bytes".  I've
> spoken
> > > with
> > > > > Nicholas about it and he doesn't like it either.  I'm hoping we can
get
> > rid
> > > of
> > > > > that stuff in the not-to-distant future, after <audio> switches to
> > > low-latency
> > > > > path and we're only using waveOut on XP.
> > > > 
> > > > I'd love to get rid of these as much as anyone else, but I don't think
> we've
> > > > found a solution that allows us to do that yet.  WebRTC requires
accurate
> > > timing
> > > > information and so does <audio> to an extent.  Even with low latency, a
> > fixed
> > > > delay is probably insufficient since buffers don't seem to stabilize (at
> > least
> > > > on Linux).
> > > > 
> > > > I don't know what kind of wrapper class we could have here aside from
> moving
> > > > this complexity into AudioBus, which I haven't fully thought through.
> > > 
> > > I was thinking of something that hides the memory layout + total size
> meaning
> > > clients only have to work with a SharedMemory* and its audio data/packet
> size
> > > and not worry about which size to pass into which functions.
> > > 
> > > class AudioSharedMemory {
> > >  public:
> > >   explicit AudioSharedMemory(handle);
> > > 
> > >   // Used by AudioRendererHost
> > >   bool CreateAndMap(audio_data_size);
> > > 
> > >   // Used by AudioOutputDevice in render process
> > >   bool Map(audio_data_size);
> > > 
> > >   // Gets our data pointer.
> > >   void* data();
> > > 
> > >   // Replaces Set/GetActualDataSizeInBytes(). These names could
> > >   // also be improved.
> > >   int actual_data_size();
> > >   void set_actual_data_size();
> > > 
> > >   // Do we still need these!?
> > >   void set_unknown_data_size();
> > >   bool is_data_size_unknown();
> > > };
> > > 
> > > Again, haven't looked at this in detail but if we'll be keeping this
around
> > for
> > > a bit longer we might as well make it less crazy.
> > > 
> > > WDYT?
> > 
> > I like it, but the old NaCl doesn't use SharedMemory or SharedMemoryHandle,
> just
> > an integer handle casted to a NaClHandle; which is why there's the
> > SetActualDataSizeInBytes(void *) version (see
> browser_ppb_audio_rpc_server.cc). 
> > They have a custom Map() process too.  Fixing the PPAPI side might be easier
> > since they at least have SharedMemory and other constructs we're familiar
> with. 
> > I don't think either fix would be simple though.
> > 
> > (Also all those methods are still needed, the unknown data size ones are
used
> > inside audio_sync_reader.)
> 
> OK -- in which case LGTM

Landing.  My hope is we can remove all this once we've got renderer side mixing.
 The pending bytes delay value is actually sent via socket and not a magic
shared memory value so only two things are necessary to remove:
1. <audio> always outputs the full frame size (bad w/ big buffers; fixed by
renderer side mixing which needs channel remixing first)
2. Deal with AudioSyncReader::Read() having no marker for how long it should
yield the audio thread when OnMoreData calls happen too quickly.

[commit-bot: I haz the power, 2012-08-29 22:50:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/10873071/1

[commit-bot: I haz the power, 2012-08-30 00:56:24]

Try job failure for 10873071-1 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...