MacOS WebM Format Reader returns enabled for tracks that do not have samples

• Component: Media
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

WebM Format Reader returns enabled for tracks that do not have samples

Makes WebGL tests fail if WebM is enabled.
WebGL test suite uses red-green.webmvp8.webm which has empty but present audio track.

[Kimmo Kinnunen]

<rdar://74048072>

[Kimmo Kinnunen]

Created attachment 429286 [details]
Patch

[Andy Estes]

Did you find where the WebGL tests started failing? Was it a WebKit commit that caused them to start failing or a change in the operating system?

Does this change fix the WebGL test failures without re-introducing a hang when reloading a page with WebM content that was fixed in r274378?

[Kimmo Kinnunen]

(In reply to Andy Estes from comment #3)
> Did you find where the WebGL tests started failing? Was it a WebKit commit
> that caused them to start failing or a change in the operating system?

Not yet. Does it matter? Is it correct to return “enabled” when we should return “disabled”? Why don’t we always return “enabled” then in the unknown-enabledness case?

> Does this change fix the WebGL test failures without re-introducing a hang
> when reloading a page with WebM content that was fixed in r274378?

It’s hard to know since no test was ever done!
I guess it’s now on me to write the test to get the bug off my priority list?
Do we have a existing case where we serve partial webm that I could start with?

If the parser cancel callback does not get run on load stop, then the hang is still there.

[Kimmo Kinnunen]

Correct radar
<rdar://77084155>

[Kimmo Kinnunen]

(In reply to Andy Estes from comment #3)
> Did you find where the WebGL tests started failing? Was it a WebKit commit
> that caused them to start failing or a change in the operating system?

The patch  here is effectively revert of r274378 so I’d imagine that to be one potential WebKit rev that caused the tests to start failing on the grounds that they start passing after this..

[Andy Estes]

(In reply to Kimmo Kinnunen from comment #4)
> (In reply to Andy Estes from comment #3)

> It’s hard to know since no test was ever done!
> I guess it’s now on me to write the test to get the bug off my priority list?

No, it's not on you to write a test; I wasn't asking you to.

> Do we have a existing case where we serve partial webm that I could start
> with?

You can try manually testing with the reproduction steps in the radar associated with r274378 (rdar://75351029).

[Kimmo Kinnunen]

(In reply to Andy Estes from comment #7)
> (In reply to Kimmo Kinnunen from comment #4)
> > It’s hard to know since no test was ever done!
> > I guess it’s now on me to write the test to get the bug off my priority list?
> No, it's not on you to write a test; I wasn't asking you to.
> You can try manually testing with the reproduction steps in the radar
> associated with r274378 (rdar://75351029).

Well it's not the kind of bug which manual reproing proves much? I could not trigger the bug manually with this patch.

I wrote the test and the hang triggers with and without this patch.

[Kimmo Kinnunen]

Created attachment 429644 [details]
Patch

[Jer Noble]

This seems like a very risky fix that could be as easily solved just by modifying the underlying WebM file to not have an audio track with no samples. After all, the failing tests are about painting to canvas, not about the robustness of our webm parser implementation. This change effectively forces the parser to parse the entire file, potentially blocked the entire time, before answering whether a given track is enabled or not.

Additionally, this won't help fix WebM files that _do_ have the enabled flag set on an audio track and yet still don't have any audio samples; those will still fail.

[Kimmo Kinnunen]

Created attachment 429751 [details]
Patch

[Kimmo Kinnunen]

(In reply to Jer Noble from comment #10)
> This seems like a very risky fix that could be as easily solved just by
> modifying the underlying WebM file to not have an audio track with no
> samples.

I don't necessarily agree that "it's as easily solved..". There's significant churn to get the changes through legal, WebGL working group and back to WebKit.

> After all, the failing tests are about painting to canvas, not
> about the robustness of our webm parser implementation. This change
> effectively forces the parser to parse the entire file, potentially blocked
> the entire time, before answering whether a given track is enabled or not.

I'd still, in my opinion, be better to work on fixing the actual problem than working on the churn while leaving the content regressed.

> Additionally, this won't help fix WebM files that _do_ have the enabled flag
> set on an audio track and yet still don't have any audio samples; those will
> still fail.

I don't necessarily understand why a non-complete implementation is a blocker in my case, but non-complete, test-regressing implementation without tests was ok in r274378?

I do understand it's better to not load some content if the alternative is a hang in other content.

[EWS]

Committed r278155 (238199@main): <https://commits.webkit.org/238199@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429751 [details].