This caused HTMLMediaElement::hasAudio() to return false for some streaming media with audio. I ran into this when watching a video on money.cnn.com.
Created attachment 240250 [details] Patch
Comment on attachment 240250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240250&action=review > LayoutTests/http/tests/media/hls/hls-audio-tracks-has-audio.html:10 > + if (window.testRunner) { > + testRunner.dumpAsText(); > + testRunner.waitUntilDone(); > + } Nit: video-test.js does this automatically so this is unnecessary. > LayoutTests/http/tests/media/hls/hls-audio-tracks-has-audio.html:12 > + function start() { Nit: the opening brace for a function is on a new line. > LayoutTests/http/tests/media/hls/hls-audio-tracks-has-audio.html:13 > + video = document.getElementById('video'); Nit: test.js automatically initializes a "video" global variable so this is unnecessary. > LayoutTests/http/tests/media/hls/hls-audio-tracks-has-audio.html:18 > + function canplaythrough() { Ditto about the function's opening brace. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1716 > + hasAudio = true; I think this should this be "|=" because it it technically possible for a file to have audio tracks that are not in a selection group. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1719 > + hasVideo = true; Ditto for video tracks. > Source/WebCore/testing/Internals.cpp:2094 > +bool Internals::mediaElementHasAudio(Node* node) > +{ > + if (!is<HTMLMediaElement>(*node)) > + return false; > + > + HTMLMediaElement* element = downcast<HTMLMediaElement>(node); > + return element->hasAudio(); > +} I would prefer it if this was more method, maybe something something like "mediaElementHasCharacteristic(Node* node, String characteristic)"
(In reply to comment #2) > Comment on attachment 240250 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=240250&action=review > > > LayoutTests/http/tests/media/hls/hls-audio-tracks-has-audio.html:10 > > + if (window.testRunner) { > > + testRunner.dumpAsText(); > > + testRunner.waitUntilDone(); > > + } > > Nit: video-test.js does this automatically so this is unnecessary. Removed. > > > LayoutTests/http/tests/media/hls/hls-audio-tracks-has-audio.html:12 > > + function start() { > > Nit: the opening brace for a function is on a new line. Fixed. > > > LayoutTests/http/tests/media/hls/hls-audio-tracks-has-audio.html:13 > > + video = document.getElementById('video'); > > Nit: test.js automatically initializes a "video" global variable so this is > unnecessary. video-test.js does initialize "video" but before the document is loaded so it may not have found it. I'm going to call findMediaElement() here instead. > > > LayoutTests/http/tests/media/hls/hls-audio-tracks-has-audio.html:18 > > + function canplaythrough() { > > Ditto about the function's opening brace. Fixed. > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1716 > > + hasAudio = true; > > I think this should this be "|=" because it it technically possible for a > file to have audio tracks that are not in a selection group. > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1719 > > + hasVideo = true; > > Ditto for video tracks. Done. > > > Source/WebCore/testing/Internals.cpp:2094 > > +bool Internals::mediaElementHasAudio(Node* node) > > +{ > > + if (!is<HTMLMediaElement>(*node)) > > + return false; > > + > > + HTMLMediaElement* element = downcast<HTMLMediaElement>(node); > > + return element->hasAudio(); > > +} > > I would prefer it if this was more method, maybe something something like > "mediaElementHasCharacteristic(Node* node, String characteristic)" Done. I'll send out a revised patch. Thanks for your feedback!
Created attachment 240294 [details] Revised patch
Created attachment 240295 [details] Revised patch: this time with the tests
Committed: http://trac.webkit.org/changeset/175072