Bug 116342 - Mac: Set the default audio buffer size to a large value for <video> elements.
Summary: Mac: Set the default audio buffer size to a large value for <video> elements.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks: 116725
  Show dependency treegraph
 
Reported: 2013-05-17 12:39 PDT by Jer Noble
Modified: 2013-05-24 11:54 PDT (History)
15 users (show)

See Also:


Attachments
Patch (35.62 KB, patch)
2013-05-17 13:11 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (36.82 KB, patch)
2013-05-17 14:48 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (41.97 KB, patch)
2013-05-19 22:39 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (41.93 KB, patch)
2013-05-19 22:58 PDT, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2013-05-17 12:39:39 PDT
Mac: Set the default audio buffer size to a large value for <video> elements.
Comment 1 Jer Noble 2013-05-17 13:11:49 PDT
Created attachment 202141 [details]
Patch
Comment 2 Build Bot 2013-05-17 13:39:33 PDT
Comment on attachment 202141 [details]
Patch

Attachment 202141 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/484804
Comment 3 EFL EWS Bot 2013-05-17 13:40:30 PDT
Comment on attachment 202141 [details]
Patch

Attachment 202141 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/487539
Comment 4 EFL EWS Bot 2013-05-17 13:41:07 PDT
Comment on attachment 202141 [details]
Patch

Attachment 202141 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/492451
Comment 5 Build Bot 2013-05-17 13:44:29 PDT
Comment on attachment 202141 [details]
Patch

Attachment 202141 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/484800
Comment 6 Early Warning System Bot 2013-05-17 14:19:24 PDT
Comment on attachment 202141 [details]
Patch

Attachment 202141 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/486563
Comment 7 Early Warning System Bot 2013-05-17 14:25:19 PDT
Comment on attachment 202141 [details]
Patch

Attachment 202141 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/492464
Comment 8 Build Bot 2013-05-17 14:39:17 PDT
Comment on attachment 202141 [details]
Patch

Attachment 202141 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/480981
Comment 9 Jer Noble 2013-05-17 14:48:52 PDT
Created attachment 202160 [details]
Patch

Added new files to the various build files for other platforms.
Comment 10 Early Warning System Bot 2013-05-17 14:53:40 PDT
Comment on attachment 202160 [details]
Patch

Attachment 202160 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/483900
Comment 11 Early Warning System Bot 2013-05-17 14:55:45 PDT
Comment on attachment 202160 [details]
Patch

Attachment 202160 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/482925
Comment 12 EFL EWS Bot 2013-05-17 15:07:46 PDT
Comment on attachment 202160 [details]
Patch

Attachment 202160 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/493427
Comment 13 Build Bot 2013-05-17 15:13:58 PDT
Comment on attachment 202160 [details]
Patch

Attachment 202160 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/497144
Comment 14 Build Bot 2013-05-17 15:25:02 PDT
Comment on attachment 202160 [details]
Patch

Attachment 202160 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/494426
Comment 15 EFL EWS Bot 2013-05-17 15:35:26 PDT
Comment on attachment 202160 [details]
Patch

Attachment 202160 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/482939
Comment 16 Build Bot 2013-05-17 16:23:03 PDT
Comment on attachment 202160 [details]
Patch

Attachment 202160 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/494442
Comment 17 kov's GTK+ EWS bot 2013-05-18 02:00:35 PDT
Comment on attachment 202160 [details]
Patch

Attachment 202160 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/499155
Comment 18 Jer Noble 2013-05-19 22:39:17 PDT
Created attachment 202258 [details]
Patch

Instead of adding what is effectively a no-op class to every port, protect non-implementing ports by wrapping AudioSession and its related classes in USE(AUDIO_SESSION) checks.
Comment 19 Jer Noble 2013-05-19 22:58:01 PDT
Created attachment 202259 [details]
Patch

Fix windows build failure.
Comment 20 Eric Carlson 2013-05-22 11:12:52 PDT
Comment on attachment 202259 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        requested buffer frame size to a large value, such as 4096. Since this results

"such as 4096" - kPreferredLowPowerBufferSize is set to 4098?

> Source/WebCore/platform/audio/AudioSessionManager.cpp:61
> +    ASSERT(type >= 0);

Is "type >= 0" really useful? Asserting that it is one of the valid values would be more complete - "ASSERT(type >= None && type <= WebAudio)"

> Source/WebCore/platform/audio/AudioSessionManager.cpp:67
> +    ASSERT(type >= 0);

Ditto.

> Source/WebCore/platform/audio/AudioSessionManager.cpp:74
> +    ASSERT(type >= 0);

Ditto.

> Source/WebCore/platform/audio/ios/AudioDestinationIOS.cpp:47
> +#if USE(AUDIO_SESSION)

Is it possible that we would ever want to build this without "AUDIO_SESSION"? If not, remove the #ifs and let the build fail if someone tries.

> Source/WebCore/platform/audio/ios/AudioSessionIOS.mm:29
> +#if USE(AUDIO_SESSION) && PLATFORM(IOS)

Ditto.

> Source/WebCore/platform/audio/mac/AudioDestinationMac.cpp:41
> +#if USE(AUDIO_SESSION)

Ditto.

> Source/WebCore/platform/audio/mac/AudioDestinationMac.cpp:74
> +#if USE(AUDIO_SESSION)
> +    return AudioSession::sharedSession().sampleRate();
> +#else
> +    return 0;
> +#endif

OTOH, if it is possible that someone would want to build this without AUDIO_SESSION shouldn't this include the old code in the #else?
Comment 21 Jer Noble 2013-05-22 11:33:26 PDT
Comment on attachment 202259 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        requested buffer frame size to a large value, such as 4096. Since this results
> 
> "such as 4096" - kPreferredLowPowerBufferSize is set to 4098?

Whoops, it should be 4096.

>> Source/WebCore/platform/audio/AudioSessionManager.cpp:61
>> +    ASSERT(type >= 0);
> 
> Is "type >= 0" really useful? Asserting that it is one of the valid values would be more complete - "ASSERT(type >= None && type <= WebAudio)"

This is really asserting that no one adds a type to the AudioType enum that is less than zero.  Because the HashMap can't handle value types that are unknown enums, i used an unsigned value (size_t), and want to guarantee no one adds an incompatible enum value.  I guess it doesn't really matter, since it will probably just cast a negative enum to a wrapped-around unsigned number.  I'll remove it.

>> Source/WebCore/platform/audio/ios/AudioDestinationIOS.cpp:47
>> +#if USE(AUDIO_SESSION)
> 
> Is it possible that we would ever want to build this without "AUDIO_SESSION"? If not, remove the #ifs and let the build fail if someone tries.

No, we will never want to use this without AUDIO_SESSION. I'll remove all these #ifs.

>> Source/WebCore/platform/audio/mac/AudioDestinationMac.cpp:74
>> +#endif
> 
> OTOH, if it is possible that someone would want to build this without AUDIO_SESSION shouldn't this include the old code in the #else?

Well, someone shouldn't do that, right? :)
Comment 22 Jer Noble 2013-05-22 16:43:13 PDT
Committed r150555: <http://trac.webkit.org/changeset/150555>
Comment 23 Ryosuke Niwa 2013-05-22 18:16:40 PDT
(In reply to comment #22)
> Committed r150555: <http://trac.webkit.org/changeset/150555>

This patch might have broken 30 media tests:
http://build.webkit.org/builders/Apple%20Lion%20Release%20WK1%20%28Tests%29/builds/12699
Comment 24 Benjamin Poulain 2013-05-22 18:18:58 PDT
Comment on attachment 202259 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:190
> +static const size_t kPreferredLowPowerBufferSize = 4098;

Where is this used?
Comment 26 Jer Noble 2013-05-22 18:53:21 PDT
(In reply to comment #24)
> (From update of attachment 202259 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202259&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:190
> > +static const size_t kPreferredLowPowerBufferSize = 4098;
> 
> Where is this used?

It was unused. I deleted this line before committing.
Comment 27 Dean Jackson 2013-05-22 20:37:16 PDT
This broke about 30 media tests so we had to roll it out: r150561
Comment 28 Jer Noble 2013-05-24 11:54:45 PDT
Committed r150651: <http://trac.webkit.org/changeset/150651>