Bug 241920 - canPlayType and video codec filtering fail for VP8 on Mac with VP8 but not VP9 hardware codecs
Summary: canPlayType and video codec filtering fail for VP8 on Mac with VP8 but not VP...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari 15
Hardware: Mac (Intel) macOS 12
: P2 Normal
Assignee: Brion Vibber
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-06-23 09:46 PDT by Brion Vibber
Modified: 2022-08-31 15:36 PDT (History)
11 users (show)

See Also:


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, Brion 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, Brion 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, Brion Vibber
no flags Details
Untested provisional patch (1.99 KB, patch)
2022-06-23 10:01 PDT, Brion 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, Brion Vibber
no flags Details | Formatted Diff | Diff
Patch (5.48 KB, patch)
2022-07-13 16:32 PDT, Brion Vibber
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brion Vibber 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=&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!
Comment 1 Brion Vibber 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.
Comment 2 Brion Vibber 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.
Comment 3 Brion Vibber 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!
Comment 4 Darin Adler 2022-06-23 10:32:57 PDT
I believe WebM is supported on all Apple Silicon Macs on macOS, but not on Intel Macs.
Comment 5 Darin Adler 2022-06-23 10:33:15 PDT
But maybe I have that wrong; can someone else confirm?
Comment 6 Brion Vibber 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.
Comment 7 Brion Vibber 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`)
Comment 8 Brion Vibber 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.)
Comment 9 Brion Vibber 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.)
Comment 10 Darin Adler 2022-06-23 10:51:49 PDT
I think you were asking for help. What help do you need?
Comment 11 Darin Adler 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?
Comment 12 Brion Vibber 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.
Comment 13 Darin Adler 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?
Comment 14 Eric Carlson 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.
Comment 15 Brion Vibber 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!
Comment 16 Youssef Soliman 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`.
Comment 17 Youssef Soliman 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
Comment 18 Brion Vibber 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!
Comment 19 Radar WebKit Bug Importer 2022-06-30 09:47:14 PDT
<rdar://problem/96226506>
Comment 20 Brion Vibber 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...
Comment 21 Brion Vibber 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)!
Comment 22 Alexey Proskuryakov 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.
Comment 23 Eric Carlson 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.
Comment 24 Brion Vibber 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!
Comment 25 Brion Vibber 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. :)
Comment 26 Youssef Soliman 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.
Comment 27 Brion Vibber 2022-07-13 16:32:21 PDT
Created attachment 460870 [details]
Patch
Comment 28 Brion Vibber 2022-07-13 16:33:05 PDT
Ok, cleaned it up now let me set it up as a PR. :D
Comment 29 Brion Vibber 2022-07-13 16:41:32 PDT
Pull request: https://github.com/WebKit/WebKit/pull/2391
Comment 30 EWS 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.