Bug 134877

Summary: [Mac] don't enable low power audio mode on external output devices
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, buildbot, commit-queue, dino, glenn, jer.noble, ossy, philipj, rniwa, sergio
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
none
This time with an actual patch sam: review+, commit-queue: commit-queue-

Description Eric Carlson 2014-07-13 19:25:33 PDT
Firewire and other "high end" output devices behave poorly when asked to use a 4K output buffer.

<rdar://17443302>
Comment 1 Eric Carlson 2014-07-13 20:50:04 PDT
Created attachment 234839 [details]
Proposed patch
Comment 2 WebKit Commit Bot 2014-07-13 20:51:34 PDT
Attachment 234839 [details] did not pass style-queue:


Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Jer Noble 2014-07-13 22:03:04 PDT
I think you may have just uploaded the ChangeLog.  Like, all of it. :)
Comment 4 Build Bot 2014-07-13 23:55:41 PDT
Comment on attachment 234839 [details]
Proposed patch

Attachment 234839 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5076455193051136

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html
Comment 5 Build Bot 2014-07-13 23:55:43 PDT
Created attachment 234842 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 6 Eric Carlson 2014-07-14 10:21:12 PDT
Created attachment 234862 [details]
This time with an actual patch
Comment 7 Jer Noble 2014-07-14 11:01:39 PDT
Comment on attachment 234862 [details]
This time with an actual patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234862&action=review

> Source/WebCore/platform/audio/mac/MediaSessionManagerMac.cpp:60
> +
> +        size_t bufferSize;
> +        if (m_audioHardwareListener && m_audioHardwareListener->outputDeviceSupportsLowPowerMode())
> +            bufferSize = kLowPowerVideoBufferSize;
> +        else
> +            bufferSize = kWebAudioBufferSize;
> +
> +        AudioSession::sharedSession().setPreferredBufferSize(bufferSize);

I think this deserves a follow-up.  The kWebAudioBufferSize value will be lower than the system default. That should be fine, as far as this bug is concerned, but we should add something later which retrieves the default size, or removes the property we set.
Comment 8 Eric Carlson 2014-07-14 11:04:10 PDT
(In reply to comment #7)
> (From update of attachment 234862 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234862&action=review
> 
> > Source/WebCore/platform/audio/mac/MediaSessionManagerMac.cpp:60
> > +
> > +        size_t bufferSize;
> > +        if (m_audioHardwareListener && m_audioHardwareListener->outputDeviceSupportsLowPowerMode())
> > +            bufferSize = kLowPowerVideoBufferSize;
> > +        else
> > +            bufferSize = kWebAudioBufferSize;
> > +
> > +        AudioSession::sharedSession().setPreferredBufferSize(bufferSize);
> 
> I think this deserves a follow-up.  The kWebAudioBufferSize value will be lower than the system default. That should be fine, as far as this bug is concerned, but we should add something later which retrieves the default size, or removes the property we set.

Good point, I will file a bug to fix this in a follow-up.
Comment 9 WebKit Commit Bot 2014-07-14 11:05:39 PDT
Comment on attachment 234862 [details]
This time with an actual patch

Rejecting attachment 234862 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 234862, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebKit2/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/6070414644084736
Comment 10 Eric Carlson 2014-07-14 11:18:23 PDT
Committed r171069: : https://trac.webkit.org/r171069
Comment 11 Csaba Osztrogonác 2014-07-14 23:01:03 PDT
(In reply to comment #10)
> Committed r171069: : https://trac.webkit.org/r171069

FYI: It broke the Apple Windows build.
Comment 12 Brent Fulgham 2014-07-15 14:18:43 PDT
Build fix committed:

Committed r171116: : https://trac.webkit.org/r171116