Audio indicator: Mac UI

Audio indicator: Mac UI

This CL hooks up the new TabAudioIndicator class to the Mac UI.

Test build: https://docs.google.com/file/d/0B0Odde3V7EhWREFSdzlEWlJIY0E/edit?usp=sharing

BUG=3541
TEST=Navigated to http://www.html5rocks.com/en/tutorials/video/basics/
Played video  and verified that an audio indicator was displayed in the tab.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188475

[Nico, 2013-03-11 02:52:56]

https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
File chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.h (right):

https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.h:19: @interface
TabAudioIndicatorViewMac : NSView {
Needs class-level comment ("// Drawn on top of favicons of tabs that play sound"
or some such)

https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
File chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.mm (right):

https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.mm:13:
TabAudioIndicatorDelegateMac(TabAudioIndicatorViewMac* view) : view_(view) {}
1-element constructors should be marked explicit

https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.mm:58:
gfx::CanvasSkiaPaint canvas(rect, false);
Hm, CanvasSkiaPaint requires allocating a skia backbuffer that's then copied to
the cocoa context in its destructor if I remember correctly. Since this is a
somewhat common operation: How %cpu does that take? Could the xplat api just
hand out a SkBitmap (which can be drawn without a duplicate copy)?

[sail, 2013-03-11 23:42:01]

https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
File chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.h (right):

https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.h:19: @interface
TabAudioIndicatorViewMac : NSView {
On 2013/03/11 02:52:56, Nico wrote:
> Needs class-level comment ("// Drawn on top of favicons of tabs that play
sound"
> or some such)

Done.

https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
File chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.mm (right):

https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.mm:13:
TabAudioIndicatorDelegateMac(TabAudioIndicatorViewMac* view) : view_(view) {}
On 2013/03/11 02:52:56, Nico wrote:
> 1-element constructors should be marked explicit

Done.

https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.mm:58:
gfx::CanvasSkiaPaint canvas(rect, false);
On 2013/03/11 02:52:56, Nico wrote:
> Hm, CanvasSkiaPaint requires allocating a skia backbuffer that's then copied
to
> the cocoa context in its destructor if I remember correctly. Since this is a
> somewhat common operation: How %cpu does that take? Could the xplat api just
> hand out a SkBitmap (which can be drawn without a duplicate copy)?

Hm.. doesn't drawing a SkBitmap require copying to a NSImage or CGImage? It not
I don't mind doing this.

Note, I'll have to manually setup the bitmap and canvas with the correct scale
factor. I'll keep the cross platform code the same and do the work in this
method.

[Nico, 2013-03-11 23:47:56]

https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
File chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.mm (right):

https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.mm:58:
gfx::CanvasSkiaPaint canvas(rect, false);
On 2013/03/11 23:42:01, sail wrote:
> On 2013/03/11 02:52:56, Nico wrote:
> > Hm, CanvasSkiaPaint requires allocating a skia backbuffer that's then copied
> to
> > the cocoa context in its destructor if I remember correctly. Since this is a
> > somewhat common operation: How %cpu does that take? Could the xplat api just
> > hand out a SkBitmap (which can be drawn without a duplicate copy)?
> 
> Hm.. doesn't drawing a SkBitmap require copying to a NSImage or CGImage? It
not
> I don't mind doing this.

My reading of SkCreateCGImageRefWithColorspace() is that it just creates a
different view on the same data, without copying pixels around. But probably
best to try locally if it makes a measurable perf difference.

> 
> Note, I'll have to manually setup the bitmap and canvas with the correct scale
> factor. I'll keep the cross platform code the same and do the work in this
> method.

[sail, 2013-03-12 00:25:11]

https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
File chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.mm (right):

https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.mm:58:
gfx::CanvasSkiaPaint canvas(rect, false);
On 2013/03/11 23:47:56, Nico wrote:
> On 2013/03/11 23:42:01, sail wrote:
> > On 2013/03/11 02:52:56, Nico wrote:
> > > Hm, CanvasSkiaPaint requires allocating a skia backbuffer that's then
copied
> > to
> > > the cocoa context in its destructor if I remember correctly. Since this is
a
> > > somewhat common operation: How %cpu does that take? Could the xplat api
just
> > > hand out a SkBitmap (which can be drawn without a duplicate copy)?
> > 
> > Hm.. doesn't drawing a SkBitmap require copying to a NSImage or CGImage? It
> not
> > I don't mind doing this.
> 
> My reading of SkCreateCGImageRefWithColorspace() is that it just creates a
> different view on the same data, without copying pixels around. But probably
> best to try locally if it makes a measurable perf difference.
> 
> > 
> > Note, I'll have to manually setup the bitmap and canvas with the correct
scale
> > factor. I'll keep the cross platform code the same and do the work in this
> > method.
> 

Hm... I think I'm doing something wrong.
The current drawing code takes ~57 micro seconds.
If I change it to draw into a SkBitmap it takes ~63 micro seconds.

Here's the code I'm using:
https://docs.google.com/file/d/0B0Odde3V7EhWc1NETFZyOGVmcEE/edit?usp=sharing

This is on a MacBook Pro Retina running on an external display.

[Nico, 2013-03-12 16:56:00]

On 2013/03/12 00:25:11, sail wrote:
>
https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
> File chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.mm (right):
> 
>
https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/ta...
> chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.mm:58:
> gfx::CanvasSkiaPaint canvas(rect, false);
> On 2013/03/11 23:47:56, Nico wrote:
> > On 2013/03/11 23:42:01, sail wrote:
> > > On 2013/03/11 02:52:56, Nico wrote:
> > > > Hm, CanvasSkiaPaint requires allocating a skia backbuffer that's then
> copied
> > > to
> > > > the cocoa context in its destructor if I remember correctly. Since this
is
> a
> > > > somewhat common operation: How %cpu does that take? Could the xplat api
> just
> > > > hand out a SkBitmap (which can be drawn without a duplicate copy)?
> > > 
> > > Hm.. doesn't drawing a SkBitmap require copying to a NSImage or CGImage?
It
> > not
> > > I don't mind doing this.
> > 
> > My reading of SkCreateCGImageRefWithColorspace() is that it just creates a
> > different view on the same data, without copying pixels around. But probably
> > best to try locally if it makes a measurable perf difference.
> > 
> > > 
> > > Note, I'll have to manually setup the bitmap and canvas with the correct
> scale
> > > factor. I'll keep the cross platform code the same and do the work in this
> > > method.
> > 
> 
> Hm... I think I'm doing something wrong.
> The current drawing code takes ~57 micro seconds.
> If I change it to draw into a SkBitmap it takes ~63 micro seconds.

How are you measuring this?

What happens if you don't allocate the SkBitmap on every frame but do that (and
probably the conversion to a cg bitmap) just once and only call the two paint
methods each frame?

> 
> Here's the code I'm using:
> https://docs.google.com/file/d/0B0Odde3V7EhWc1NETFZyOGVmcEE/edit?usp=sharing
> 
> This is on a MacBook Pro Retina running on an external display.

[Nico, 2013-03-12 16:57:01]

9Re "How are you measuring this?": Since the window server might delay some
things until paint time, it might be more useful to look at %cpu while the
animation is playing than measuring time in the drawing code.)


On Tue, Mar 12, 2013 at 9:56 AM, <thakis@chromium.org> wrote:

> On 2013/03/12 00:25:11, sail wrote:
>
> https://codereview.chromium.**org/12648004/diff/2001/chrome/**
>
browser/ui/cocoa/tabs/tab_**audio_indicator_view_mac.mm<https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.mm>
>
>> File
chrome/browser/ui/cocoa/tabs/t**ab_audio_indicator_view_mac.mm<http://tab_audio_indicator_view_mac.mm>(right):
>>
>
>
> https://codereview.chromium.**org/12648004/diff/2001/chrome/**
>
browser/ui/cocoa/tabs/tab_**audio_indicator_view_mac.mm#**newcode58<https://codereview.chromium.org/12648004/diff/2001/chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.mm#newcode58>
>
>>
chrome/browser/ui/cocoa/tabs/t**ab_audio_indicator_view_mac.**mm:58<http://tab_audio_indicator_view_mac.mm:58>
>> :
>> gfx::CanvasSkiaPaint canvas(rect, false);
>> On 2013/03/11 23:47:56, Nico wrote:
>> > On 2013/03/11 23:42:01, sail wrote:
>> > > On 2013/03/11 02:52:56, Nico wrote:
>> > > > Hm, CanvasSkiaPaint requires allocating a skia backbuffer that's
>> then
>> copied
>> > > to
>> > > > the cocoa context in its destructor if I remember correctly. Since
>> this
>>
> is
>
>> a
>> > > > somewhat common operation: How %cpu does that take? Could the xplat
>> api
>> just
>> > > > hand out a SkBitmap (which can be drawn without a duplicate copy)?
>> > >
>> > > Hm.. doesn't drawing a SkBitmap require copying to a NSImage or
>> CGImage?
>>
> It
>
>> > not
>> > > I don't mind doing this.
>> >
>> > My reading of SkCreateCGImageRefWithColorspa**ce() is that it just
>> creates a
>> > different view on the same data, without copying pixels around. But
>> probably
>> > best to try locally if it makes a measurable perf difference.
>> >
>> > >
>> > > Note, I'll have to manually setup the bitmap and canvas with the
>> correct
>> scale
>> > > factor. I'll keep the cross platform code the same and do the work in
>> this
>> > > method.
>> >
>>
>
>  Hm... I think I'm doing something wrong.
>> The current drawing code takes ~57 micro seconds.
>> If I change it to draw into a SkBitmap it takes ~63 micro seconds.
>>
>
> How are you measuring this?
>
> What happens if you don't allocate the SkBitmap on every frame but do that
> (and
> probably the conversion to a cg bitmap) just once and only call the two
> paint
> methods each frame?
>
>
>
>  Here's the code I'm using:
>> https://docs.google.com/file/**d/**0B0Odde3V7EhWc1NETFZyOGVmcEE/**
>>
edit?usp=sharing<https://docs.google.com/file/d/0B0Odde3V7EhWc1NETFZyOGVmcEE/edit?usp=sharing>
>>
>
>  This is on a MacBook Pro Retina running on an external display.
>>
>
>
>
>
https://codereview.chromium.**org/12648004/<https://codereview.chromium.org/1...
>

[sail, 2013-03-13 00:31:57]

> How are you measuring this?

I was using  base::TimeTicks::HighResNow to get the start and end time.

> What happens if you don't allocate the SkBitmap on every frame but do that
(and
> probably the conversion to a cg bitmap) just once and only call the two paint
> methods each frame?

I tried allocating this and I couldn't get the CGImage to update. It looks like
maybe it's doing some kind of copying. Here's what I'm trying:
https://docs.google.com/file/d/0B0Odde3V7EhWUWZieDBNTFVWSnc/edit?usp=sharing

> 9Re "How are you measuring this?": Since the window server might delay some
> things until paint time, it might be more useful to look at %cpu while the
> animation is playing than measuring time in the drawing code.)

I tried measuring CPU usage. Here's what I did:
  - comment out the printfs
  - force audio indicator to show
  - navigate to about:blank
  - click on activity monitor
For both the CanvasSkiaPaint code and the SkBitmap code the CPU usage stays
between 2.4% and 2.5%.

[Nico, 2013-03-13 01:03:12]

What if you allocate the skbitmap just once but get a new cgbitmap every
time?

Sounds like it won't matter, but should only take 10s to try.


On Tue, Mar 12, 2013 at 5:31 PM, <sail@chromium.org> wrote:

> How are you measuring this?
>>
>
> I was using  base::TimeTicks::HighResNow to get the start and end time.
>
>
>  What happens if you don't allocate the SkBitmap on every frame but do that
>>
> (and
>
>> probably the conversion to a cg bitmap) just once and only call the two
>> paint
>> methods each frame?
>>
>
> I tried allocating this and I couldn't get the CGImage to update. It looks
> like
> maybe it's doing some kind of copying. Here's what I'm trying:
> https://docs.google.com/file/**d/**0B0Odde3V7EhWUWZieDBNTFVWSnc/**
>
edit?usp=sharing<https://docs.google.com/file/d/0B0Odde3V7EhWUWZieDBNTFVWSnc/edit?usp=sharing>
>
>
>  9Re "How are you measuring this?": Since the window server might delay
>> some
>> things until paint time, it might be more useful to look at %cpu while the
>> animation is playing than measuring time in the drawing code.)
>>
>
> I tried measuring CPU usage. Here's what I did:
>   - comment out the printfs
>   - force audio indicator to show
>   - navigate to about:blank
>   - click on activity monitor
> For both the CanvasSkiaPaint code and the SkBitmap code the CPU usage stays
> between 2.4% and 2.5%.
>
>
>
https://codereview.chromium.**org/12648004/<https://codereview.chromium.org/1...
>

[sail, 2013-03-13 01:16:06]

On 2013/03/13 01:03:12, Nico wrote:
> What if you allocate the skbitmap just once but get a new cgbitmap every
> time?
> 
> Sounds like it won't matter, but should only take 10s to try.

CPU usage was the same. Here's the code I tried:
https://docs.google.com/file/d/0B0Odde3V7EhWamRnX29ZeGFWTlU/edit?usp=sharing

[sail, 2013-03-13 01:19:32]

On 2013/03/13 01:16:06, sail wrote:
> On 2013/03/13 01:03:12, Nico wrote:
> > What if you allocate the skbitmap just once but get a new cgbitmap every
> > time?
> > 
> > Sounds like it won't matter, but should only take 10s to try.
> 
> CPU usage was the same. Here's the code I tried:
> https://docs.google.com/file/d/0B0Odde3V7EhWamRnX29ZeGFWTlU/edit?usp=sharing

By the way, if I change the frame rate from 15 to 1 I get about 0.5% CPU usage.
We don't actually draw at 15FPS because many times the equalizer levels don't
change so there's nothing new to draw.

Even so, I think we can improve perf here by reducing the number of levels from
8 to 4. This should cut our actual frame rate in half.

[Nico, 2013-03-14 21:03:45]

lgtm

[commit-bot: I haz the power, 2013-03-14 21:15:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/12648004/8001

[commit-bot: I haz the power, 2013-03-14 21:15:32]

Failed to apply patch for chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
  Hunk #2 FAILED at 112.
  Hunk #3 succeeded at 1573 (offset -3 lines).
  Hunk #4 FAILED at 1611.
  2 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm.rej

Patch:       chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
Index: chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
diff --git a/chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
b/chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
index
17c28f8094acac4689326f1d81ae9868e742dab9..31d2130114d5e5b85860e705b53e125bfa779e98
100644
--- a/chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
+++ b/chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm
@@ -37,6 +37,7 @@
 #import "chrome/browser/ui/cocoa/new_tab_button.h"
 #import "chrome/browser/ui/cocoa/tab_contents/favicon_util_mac.h"
 #import "chrome/browser/ui/cocoa/tab_contents/tab_contents_controller.h"
+#import "chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.h"
 #import "chrome/browser/ui/cocoa/tabs/tab_controller.h"
 #import "chrome/browser/ui/cocoa/tabs/tab_projecting_image_view.h"
 #import "chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.h"
@@ -111,9 +112,6 @@ const CGFloat kProjectingIconWidthAndHeight = 32.0;
 // Throbbing duration on webrtc "this web page is watching you" favicon
overlay.
 const int kRecordingDurationMs = 1000;
 
-// Throbbing duration on audio playing animation.
-const int kAudioPlayingDurationMs = 2000;
-
 // Helper class for doing NSAnimationContext calls that takes a bool to disable
 // all the work.  Useful for code that wants to conditionally animate.
 class ScopedNSAnimationContextGroup {
@@ -1575,6 +1573,9 @@ NSImage* Overlay(NSImage* ground, NSImage* overlay,
CGFloat alpha) {
     if (newHasIcon) {
       if (newState == kTabDone) {
         NSImageView* imageView = [self iconImageViewForContents:contents];
+        TabAudioIndicatorViewMac* tabAudioIndicatorViewMac =
+            base::mac::ObjCCast<TabAudioIndicatorViewMac>(
+                [tabController iconView]);
 
         ui::ThemeProvider* theme = [[tabStripView_ window] themeProvider];
         if (theme && [tabController projecting]) {
@@ -1607,20 +1608,18 @@ NSImage* Overlay(NSImage* ground, NSImage* overlay,
CGFloat alpha) {
                   autorelease];
 
           iconView = recordingView;
-        } else if (theme && chrome::IsPlayingAudio(contents)) {
-          NSImage* audioImage =
-              theme->GetNSImageNamed(IDR_AUDIO_ANIMATION, true);
-          NSRect frame =
-              NSMakeRect(0, 0, kIconWidthAndHeight, kIconWidthAndHeight);
-          ThrobbingImageView* equalizerFaviconView =
-              [[[ThrobbingImageView alloc]
-                  initWithFrame:frame
-                backgroundImage:[imageView image]
-                     throbImage:audioImage
-                     durationMS:kAudioPlayingDurationMs] autorelease];
-          [equalizerFaviconView setTweenType:ui::Tween::LINEAR];
-
-          iconView = equalizerFaviconView;
+        } else if (chrome::IsPlayingAudio(contents) ||
+                   [tabAudioIndicatorViewMac isAnimating]) {
+          if (!tabAudioIndicatorViewMac) {
+            NSRect frame =
+                NSMakeRect(0, 0, kIconWidthAndHeight, kIconWidthAndHeight);
+            tabAudioIndicatorViewMac = [[[TabAudioIndicatorViewMac alloc]
+                initWithFrame:frame] autorelease];
+          }
+          [tabAudioIndicatorViewMac
+              setIsPlayingAudio:chrome::IsPlayingAudio(contents)];
+          [tabAudioIndicatorViewMac setBackgroundImage:[imageView image]];
+          iconView = tabAudioIndicatorViewMac;
         } else {
           iconView = imageView;
         }

[commit-bot: I haz the power, 2013-03-15 17:22:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/12648004/22001

[commit-bot: I haz the power, 2013-03-15 18:31:33]

Retried try job too often on mac_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-03-15 18:38:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/12648004/22001

[commit-bot: I haz the power, 2013-03-15 18:49:17]

Retried try job too often on mac_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-03-15 19:05:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/12648004/22001

[commit-bot: I haz the power, 2013-03-15 21:06:47]

Change committed as 188475