Bug 213947

Summary: [GStreamer] media/vp9.html failing since check-in in r263894
Product: WebKit Reporter: Lauro Moura <lmoura>
Component: MediaAssignee: Xabier Rodríguez Calvar <calvaris>
Status: RESOLVED FIXED    
Severity: Normal CC: aperez, bugs-noreply, calvaris, darin, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, pnormand, sergio, vjaquez, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=213780
Attachments:
Description Flags
Patch
none
Patch
none
Patch
pnormand: review+
Patch for landing none

Description Lauro Moura 2020-07-03 19:04:23 PDT
Like the webrtc counterpart in bug213780, media/vp9.html is failing since added:

--- /home/buildbot/worker/gtk-linux-64-release-tests/build/layout-test-results/media/vp9-expected.txt
+++ /home/buildbot/worker/gtk-linux-64-release-tests/build/layout-test-results/media/vp9-actual.txt
@@ -1,5 +1,5 @@
 
 
-PASS HTMLMediaElement.canPlay VP9 
+FAIL HTMLMediaElement.canPlay VP9 assert_equals: canPlayType expected "maybe" but got "probably"
 PASS VP9 decoding
Comment 1 Philippe Normand 2020-07-04 01:15:28 PDT
I don't think 2 bugs are needed.

*** This bug has been marked as a duplicate of bug 213780 ***
Comment 2 Philippe Normand 2020-07-04 01:16:09 PDT
Oh it's 2 different tests. My bad :)
Comment 3 Philippe Normand 2020-07-08 04:15:05 PDT
media/vp9.html has this assert:

assert_equals(video.canPlayType("video/mp4; codecs=vp9"), "maybe", "canPlayType");

Youenn, in GTK/WPE this returns "probably". Would it be OK to update the test changing the assert to a less restrictive test? Perhaps simply log the result in test expectation?
Comment 4 Xabier Rodríguez Calvar 2020-07-08 23:00:11 PDT
(In reply to Philippe Normand from comment #3)
> media/vp9.html has this assert:
> 
> assert_equals(video.canPlayType("video/mp4; codecs=vp9"), "maybe",
> "canPlayType");
> 
> Youenn, in GTK/WPE this returns "probably". Would it be OK to update the
> test changing the assert to a less restrictive test? Perhaps simply log the
> result in test expectation?

From what I read in the spec at [1], it's clear that we're specifying the codec here so a user agent is per spec allowed to return probably.

[1] https://html.spec.whatwg.org/multipage/media.html#dom-navigator-canplaytype

This makes this test very highly dependent on the platform so I'd say we have to log the result and make different expectations for platforms. You can leave a general one with maybe and another glib one with probably.
Comment 5 Philippe Normand 2020-07-09 02:04:20 PDT
Also exiting early if !window.internals.usingAppleInternalSDK is a bit sad, for non-Apple ports :(
Comment 6 youenn fablet 2020-07-20 06:15:58 PDT
(In reply to Philippe Normand from comment #5)
> Also exiting early if !window.internals.usingAppleInternalSDK is a bit sad,
> for non-Apple ports :(
(In reply to Philippe Normand from comment #3)
> media/vp9.html has this assert:
> 
> assert_equals(video.canPlayType("video/mp4; codecs=vp9"), "maybe",
> "canPlayType");
> 
> Youenn, in GTK/WPE this returns "probably". Would it be OK to update the
> test changing the assert to a less restrictive test? Perhaps simply log the
> result in test expectation?

I think it is fine to check for either maybe or probably.

(In reply to Philippe Normand from comment #5)
> Also exiting early if !window.internals.usingAppleInternalSDK is a bit sad,
> for non-Apple ports :(

We could probably rewrite the test so that rejection of video.play() is tolerated in case of non Apple Internal SDK.
Comment 7 Xabier Rodríguez Calvar 2020-07-27 08:39:13 PDT
Created attachment 405278 [details]
Patch
Comment 8 Adrian Perez 2020-07-27 09:05:12 PDT
Comment on attachment 405278 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        * media/vp9.html: Non Apple ports return propably instead of ""

Typo: propably → probably
Comment 9 Xabier Rodríguez Calvar 2020-07-27 09:25:57 PDT
Created attachment 405285 [details]
Patch

Add isApple which gives better granularity and allows GStreamer to run the second test.
Comment 10 Darin Adler 2020-07-27 12:10:56 PDT
Comment on attachment 405285 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:5531
> +#if PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(MACCATALYST)
> +    return true;
> +#else
> +    return false;
> +#endif

This is an incorrect implementation. Will return false for watchOS and tvOS. Simpler is to just write PLATFORM(COCOA).

Not thrilled with the name of this. It seems imprecise. It will return false if you are running on Apple hardware but with a non-Apple port of WebKIt, and will return false if you are running on Windows with an Apple port of WebKit.
Comment 11 Xabier Rodríguez Calvar 2020-07-27 22:14:31 PDT
(In reply to Darin Adler from comment #10)
> Comment on attachment 405285 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405285&action=review
> 
> > Source/WebCore/testing/Internals.cpp:5531
> > +#if PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(MACCATALYST)
> > +    return true;
> > +#else
> > +    return false;
> > +#endif
> 
> This is an incorrect implementation. Will return false for watchOS and tvOS.
> Simpler is to just write PLATFORM(COCOA).

Ok,
 
> Not thrilled with the name of this. It seems imprecise. It will return false
> if you are running on Apple hardware but with a non-Apple port of WebKIt,
> and will return false if you are running on Windows with an Apple port of
> WebKit.

I'll name it isApplePort.
Comment 12 Xabier Rodríguez Calvar 2020-07-28 01:02:32 PDT
Created attachment 405344 [details]
Patch

Implemented with internals.usingGStreamer()
Comment 13 Xabier Rodríguez Calvar 2020-07-28 04:47:51 PDT
Created attachment 405350 [details]
Patch for landing
Comment 14 EWS 2020-07-28 05:18:11 PDT
Committed r264975: <https://trac.webkit.org/changeset/264975>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405350 [details].
Comment 15 Radar WebKit Bug Importer 2020-07-28 05:19:22 PDT
<rdar://problem/66212906>