Bug 116342 – Mac: Set the default audio buffer size to a large value for <video> elements.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Mac: Set the default audio buffer size to a large value for <video> elements.

[Jer Noble]

Created attachment 202141 [details]
Patch

[Build Bot]

Comment on attachment 202141 [details]
Patch

Attachment 202141 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/484804

[EFL EWS Bot]

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

[EFL EWS Bot]

Comment on attachment 202141 [details]
Patch

Attachment 202141 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/492451

[Build Bot]

Comment on attachment 202141 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 202141 [details]
Patch

Attachment 202141 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/486563

[Early Warning System Bot]

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

[Build Bot]

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

[Jer Noble]

Created attachment 202160 [details]
Patch

Added new files to the various build files for other platforms.

[Early Warning System Bot]

Comment on attachment 202160 [details]
Patch

Attachment 202160 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/483900

[Early Warning System Bot]

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

[EFL EWS Bot]

Comment on attachment 202160 [details]
Patch

Attachment 202160 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/493427

[Build Bot]

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

[Build Bot]

Comment on attachment 202160 [details]
Patch

Attachment 202160 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/494426

[EFL EWS Bot]

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

[Build Bot]

Comment on attachment 202160 [details]
Patch

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

[kov's GTK+ EWS bot]

Comment on attachment 202160 [details]
Patch

Attachment 202160 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/499155

[Jer Noble]

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.

[Jer Noble]

Created attachment 202259 [details]
Patch

Fix windows build failure.

[Eric Carlson]

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?

[Jer Noble]

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? :)

[Jer Noble]

Committed r150555: <http://trac.webkit.org/changeset/150555>

[Ryosuke Niwa]

(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

[Benjamin Poulain]

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?

[Ryosuke Niwa]

WK2 failures: http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r150555%20(9117)/results.html

[Jer Noble]

(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.

[Dean Jackson]

This broke about 30 media tests so we had to roll it out: r150561

[Jer Noble]

Committed r150651: <http://trac.webkit.org/changeset/150651>