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

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Brooke Vibber]

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=&quot;vp8, vorbis&quot;" 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!

[Brooke Vibber]

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]

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]

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]

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

[Darin Adler]

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

[Brooke Vibber]

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]

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

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

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

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

[Darin Adler]

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]

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

[Darin Adler]

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]

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

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]

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]

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

[Brooke Vibber]

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

<rdar://problem/96226506>

[Brooke Vibber]

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]

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]

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]

> 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]

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

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 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]

Created attachment 460870 [details]
Patch

[Brooke Vibber]

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

[Brooke Vibber]

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

[EWS]

Committed 254013@main (5706f1c79429): <https://commits.webkit.org/254013@main>

Reviewed commits have been landed. Closing PR #2391 and removing active labels.