Summary: | [GStreamer] media/vp9.html failing since check-in in r263894 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Lauro Moura <lmoura> | ||||||||||
Component: | Media | Assignee: | 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
Lauro Moura
2020-07-03 19:04:23 PDT
I don't think 2 bugs are needed. *** This bug has been marked as a duplicate of bug 213780 *** Oh it's 2 different tests. My bad :) 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? (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. Also exiting early if !window.internals.usingAppleInternalSDK is a bit sad, for non-Apple ports :( (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. Created attachment 405278 [details]
Patch
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 Created attachment 405285 [details]
Patch
Add isApple which gives better granularity and allows GStreamer to run the second test.
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. (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. Created attachment 405344 [details]
Patch
Implemented with internals.usingGStreamer()
Created attachment 405350 [details]
Patch for landing
Committed r264975: <https://trac.webkit.org/changeset/264975> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405350 [details]. |