Bug 137935 – MediaPlayerPrivateAVFoundation::hasAudio() returns false even when there is an audible AVMediaSelectionOption selected

• Component: Media
• Status: RESOLVED FIXED

[Ada Chan]

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.

[Ada Chan]

Created attachment 240250 [details]
Patch

[Eric Carlson]

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)"

[Ada Chan]

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

[Ada Chan]

Created attachment 240294 [details]
Revised patch

[Ada Chan]

Created attachment 240295 [details]
Revised patch: this time with the tests

[Ada Chan]

Committed:
http://trac.webkit.org/changeset/175072