Bug 241920

Summary:

canPlayType and video codec filtering fail for VP8 on Mac with VP8 but not VP9 hardware codecs

Product:

WebKit

Reporter:

Brooke Vibber <bvibber>

Component:

Media

Assignee:

Brooke Vibber <bvibber>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, darin, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, webkit-bug-importer, youssefdevelops, y_soliman

Priority:

Keywords:

InRadar

Version:

Safari 15

Hardware:

Mac (Intel)

OS:

macOS 12

Attachments:

Description	Flags
Screenshot of the linked/described examples: the top with full codec description fails, while the bottom works. If you try this on a newer Mac with hardware VP9 it will load both videos.	none
Corrected screenshot - shows the failure case at top (marked as vp8 codec) which works on later Macs	none
Actually corrected screenshot - top video fails unexpectedly, bottom works as expected.	none
Untested provisional patch	ews-feeder: commit-queue-
Updated patch; confirmed working, but test case gives false positive even if the fix is removed	none
Patch	none

Brooke Vibber

Reported 2022-06-23 09:46:59 PDT

Created attachment 460445 [details] Screenshot of the linked/described examples: the top with full codec description fails, while the bottom works. If you try this on a newer Mac with hardware VP9 it will load both videos. On an early-2015 Retina MacBook Pro 13" running macOS 12.4 and both current Safari and Technology Preview, HTMLMediaElement.prototype.canPlayType and the filtering of <video> <source>s based on codecs in the type attribute indicate that both VP9 and VP8 are not supported, however: 1) if you don't specify a codec, and just specify 'video/webm', it does play 2) the CPU is an Intel Core i7-5557U, which has Intel Iris Graphics 6100, which includes a hardware VP8 decoder, so it *should* work unless macOS doesn't support it -- in which case 1) would not happen Example URL: https://brionv.com/misc/hls-test/vp8-webm.html Key bits being this fails: <video controls width=640 height=360> <source type="video/webm; codecs="vp8, vorbis"" src=polyphon-vp8-vorbis.webm> </video> but this works: <p>WebM VP8/Vorbis marked as <code>video/webm</code>:</p> <video controls width=640 height=360> <source type="video/webm" src=polyphon-vp8-vorbis.webm> </video> on the affected device. I've been trying to make a local WebKit build to see if I can patch it, however no local build I produce actually seems to support WebM video -- is there a special parameter I need to be passing to the build script? As far as I can tell the ENABLE_VP9, ENABLE_VORBIS, etc are switched on thanks to building for Mac with a recent SDK? Is there anything else that needs to be done? Thanks!

Attachments
Screenshot of the linked/described examples: the top with full codec description fails, while the bottom works. If you try this on a newer Mac with hardware VP9 it will load both videos. (3.10 MB, image/png) 2022-06-23 09:46 PDT, Brooke Vibber	no flags	Details
Corrected screenshot - shows the failure case at top (marked as vp8 codec) which works on later Macs (3.10 MB, image/png) 2022-06-23 09:52 PDT, Brooke Vibber	no flags	Details
Actually corrected screenshot - top video fails unexpectedly, bottom works as expected. (995.08 KB, image/png) 2022-06-23 09:54 PDT, Brooke Vibber	no flags	Details
Untested provisional patch (1.99 KB, patch) 2022-06-23 10:01 PDT, Brooke Vibber	ews-feeder: commit-queue-	Details Formatted Diff Diff
Updated patch; confirmed working, but test case gives false positive even if the fix is removed (5.84 KB, patch) 2022-07-07 10:22 PDT, Brooke Vibber	no flags	Details Formatted Diff Diff
Patch (5.48 KB, patch) 2022-07-13 16:32 PDT, Brooke Vibber	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Brooke Vibber

Comment 1 2022-06-23 09:52:55 PDT

Created attachment 460446 [details] Corrected screenshot - shows the failure case at top (marked as vp8 codec) which works on later Macs Sorry, wrong image -- corrected.

Brooke Vibber

Comment 2 2022-06-23 09:54:28 PDT

Created attachment 460447 [details] Actually corrected screenshot - top video fails unexpectedly, bottom works as expected. The upload gods are not on my side today. ;) Fixed image.

Brooke Vibber

Comment 3 2022-06-23 10:01:50 PDT

Created attachment 460448 [details] Untested provisional patch I put together this patch the other day after finding what looks like a suspicious ordering of operations, however I'm unable to test it because my WebKit builds don't appear to support WebM video files at all. I would appreciate help with this, such as confirmation of the bug, the functionality of the patch, and how to build WebKit with WebM support on macOS. Thanks!

Darin Adler

Comment 4 2022-06-23 10:32:57 PDT

I believe WebM is supported on all Apple Silicon Macs on macOS, but not on Intel Macs.

Darin Adler

Comment 5 2022-06-23 10:33:15 PDT

But maybe I have that wrong; can someone else confirm?

Brooke Vibber

Comment 6 2022-06-23 10:36:41 PDT

That would be surprising, as WebM with VP8 and VP9 and Vorbis and Opus all work on a 2018 MacBook Pro with Core i9.

Brooke Vibber

Comment 7 2022-06-23 10:38:34 PDT

(In reply to Brion Vibber from comment #6) > That would be surprising, as WebM with VP8 and VP9 and Vorbis and Opus all > work on a 2018 MacBook Pro with Core i9. (but I should point out -- they work in Safari and Safari Technology Preview, *not* in `run-safari` after `build-webkit`)

Brooke Vibber

Comment 8 2022-06-23 10:40:47 PDT

(If you mean "not on all Intel macs" and not "not on any Intel macs" then you are correct, as hardware codec support is required.)

Brooke Vibber

Comment 9 2022-06-23 10:44:37 PDT

(To make sure this is not lost: WebM and hardware VP8 decoding are already supported on the target machine by Safari and it plays back correctly *unless* it's labeled as VP8 in the type string.)

Darin Adler

Comment 10 2022-06-23 10:51:49 PDT

I think you were asking for help. What help do you need?

Darin Adler

Comment 11 2022-06-23 10:52:32 PDT

Oh, your problem is that *your own* WebKit builds don’t work with WebM and you don’t know why. That is what you need help debugging?

Brooke Vibber

Comment 12 2022-06-23 11:03:51 PDT

Correct. I can't test the patch because local builds don't have the same features as Apple's builds.

Darin Adler

Comment 13 2022-06-23 16:58:41 PDT

Jer, Eric, any advice on how Brion can test this? He’s saying "Apple’s builds" have this different, but that surprises me. What part of this depends on something specific to Apple builds?

Eric Carlson

Comment 14 2022-06-23 17:42:56 PDT

(In reply to Darin Adler from comment #13) > Jer, Eric, any advice on how Brion can test this? He’s saying "Apple’s > builds" have this different, but that surprises me. What part of this > depends on something specific to Apple builds? Unfortunately this is a restriction of the SPI we use to create the plug-in that hooks our WebM parser into AVFoundation, which requires that the plug-in be code-signing with a specific certificate. @y_soliman is working on a different approach that doesn't require the use of the plug-in, but it isn't ready yet.

Brooke Vibber

Comment 15 2022-06-27 08:57:59 PDT

Thanks -- sounds like until that gets worked out I won't be able test the patch myself in the near term, so I entrust it into you folks' hands. :) Let me know if I can provide any more detail, sample files etc. Thanks!

Youssef Soliman

Comment 16 2022-06-28 14:27:34 PDT

Hey Brion, I just tested the URL you sent on an M1 MacBook and all of the videos were able to load properly. Not sure what is going wrong there. The patch for the alternate WebM player landed on the main branch recently so if you would like to test this patch locally, you should be able to turn on the "Alternate WebM Player" internal feature flag (Settings -> Internal Features) within a freshly built mini-browser `run-minibrowser`.

Youssef Soliman

Comment 17 2022-06-28 14:30:57 PDT

I should also add that vorbis decoding is currently broken tracked here: https://bugs.webkit.org/show_bug.cgi?id=242040

Brooke Vibber

Comment 18 2022-06-28 15:36:16 PDT

(In reply to Youssef Soliman from comment #16) > Hey Brion, I just tested the URL you sent on an M1 MacBook and all of the > videos were able to load properly. Not sure what is going wrong there. An M1 MacBook will indeed play them all. The bug occurs on hardware that supports VP8, but not VP9, such as an early-2015 MacBook Pro 13". > The patch for the alternate WebM player landed on the main branch recently > so if you would like to test this patch locally, you should be able to turn > on the "Alternate WebM Player" internal feature flag (Settings -> Internal > Features) within a freshly built mini-browser `run-minibrowser`. Thanks, I'll give that a try!

Radar WebKit Bug Importer

Comment 19 2022-06-30 09:47:14 PDT

<rdar://problem/96226506>

Brooke Vibber

Comment 20 2022-07-01 08:39:48 PDT

I can confirm I see the bug on the early-2015 MacBook Pro 13" with the "alternate WebM player" selected (had to restart after picking it in the menu). The VP8 video plays when labeled as 'video/webm' but not when labeled as 'video/webm; codecs="vp8, vorbis"' or 'video/webm; codecs="vp8"'. At the moment the vorbis audio doesn't play but that's a separate issue. :) Will test the provisional patch while I'm in here...

Brooke Vibber

Comment 21 2022-07-01 08:52:15 PDT

Ok, can confirm that my provisional patch gets the no-audio version working (type='video/webm; codecs="vp8"'). canPlayType and isTypeSupported also return 'probably' and true, respectively. Yay! :) (The one with Vorbis audio listed is only failing now because of the Vorbis problem in the alternate webm player, and should be fixed along with it.) I'd be happy to sign any necessary copyright assignment, or feel free to recreate the patch (it's very simple)!

Alexey Proskuryakov

Comment 22 2022-07-01 10:00:36 PDT

We don't require copyright assignment. As long as your contribution is licensed under BSD license, you can just add your copyright line to this file. The patch cannot be landed as is, because its commit message doesn't conform to the format. Would you be willing to regenerate it? Please see https://webkit.org/contributing-code/ I have no insight into the substance of the fix, someone else would need to comment about that.

Eric Carlson

Comment 23 2022-07-01 10:58:43 PDT

> I have no insight into the substance of the fix, someone else would need to comment about that. The code change looks good to me, although it would be nice to have a test. A WebKit layout test can enable and disable the VP9 HW decoder with `internals.setHardwareVP9DecoderDisabledForTesting` to , see LayoutTests/platform/mac/media/mediacapabilities/vp9-decodingInfo-sw.html for example. This could be used to disable the HW decoder and exercise your logic. If you don't have time to create a test we will pick up your work and create a full PR in the next week or so.

Brooke Vibber

Comment 24 2022-07-01 19:30:32 PDT

(In reply to Eric Carlson from comment #23) > > I have no insight into the substance of the fix, someone else would need to comment about that. > > The code change looks good to me, although it would be nice to have a test. Thanks, I'll see if I can get a test running & clean up the commit comment!

Brooke Vibber

Comment 25 2022-07-07 10:22:52 PDT

Created attachment 460741 [details] Updated patch; confirmed working, but test case gives false positive even if the fix is removed Updated patch, manually confirmed working under alternative WebM player: * proper commit message format * changed isVP9DecoderAvailable() to respect the vp9 testing disable override * test case However, the test case passes if the actual fix is removed -- it doesn't seem to be using the Cocoa SourceBufferParserWebM from what I can tell, and I'm not sure if it's possible to enable the alternative WebM backend at runtime in a test, or even enable it globally for the test run. I might just be missing a key bit in setting up the settings for the tests, if so I'm happy to do a final rev on the patch. :)

Youssef Soliman

Comment 26 2022-07-07 11:26:16 PDT

Comment on attachment 460741 [details] Updated patch; confirmed working, but test case gives false positive even if the fix is removed View in context: https://bugs.webkit.org/attachment.cgi?id=460741&action=review The fix looks good to me, I recommend following the instructions here: https://github.com/WebKit/WebKit/blob/main/Introduction.md#contributing-code-to-webkit for our new GitHub workflow for landing changes. You should be able to just undo the commit with `git reset HEAD~1` (assuming this is the latest commit on your working tree), and run `git-webkit pr` to bring these changes to a GitHub PR. > COMMIT_MESSAGE:8 > +SourceBufferParserWebM::isContentTypeSupported called > +isVP9DecoderAvailable for both VP9 and VP8 codecs, which caused > +some older Macs to misreport a failure of VP8 support in > +HTMLMediaElement.canPlayType, MediaSource.isTypeSupported, and > +the filtering of video sources based on codec lists. > +https://bugs.webkit.org/show_bug.cgi?id=241920 You can refer to the format of commit messages here: https://github.com/WebKit/WebKit/blob/main/Introduction.md#commit-messages > LayoutTests/platform/mac/media/media-can-play-vp8-expected.txt:7 > +EXPECTED (video.canPlayType('video/webm') == 'maybe'), OBSERVED '' FAIL > +EXPECTED (video.canPlayType('video/webm; codecs=vp8') == 'probably'), OBSERVED '' FAIL It seems like the expected test case is the failing case. Try deleting the `-expected.txt` file and re-running the test runner with the flag `--internal-feature AlternateWebMPlayerEnabled=true` and verify that the expected case is a pass.

Brooke Vibber

Comment 27 2022-07-13 16:32:21 PDT

Created attachment 460870 [details] Patch

Brooke Vibber

Comment 28 2022-07-13 16:33:05 PDT

Ok, cleaned it up now let me set it up as a PR. :D

Brooke Vibber

Comment 29 2022-07-13 16:41:32 PDT

Pull request: https://github.com/WebKit/WebKit/pull/2391

EWS

Comment 30 2022-08-31 15:36:46 PDT

Committed 254013@main (5706f1c79429): <https://commits.webkit.org/254013@main> Reviewed commits have been landed. Closing PR #2391 and removing active labels.

Note You need to log in before you can comment on or make changes to this bug.