Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove WebCodecs tests with PRECONDITION_FAILED #375

Closed
nt1m opened this issue Jul 7, 2023 · 34 comments
Closed

Remove WebCodecs tests with PRECONDITION_FAILED #375

nt1m opened this issue Jul 7, 2023 · 34 comments
Labels
focus area: Web Codecs (video) test-change-proposal Proposal to add or remove tests for an interop area

Comments

@nt1m
Copy link
Member

nt1m commented Jul 7, 2023

Test List

  • ✅ full-cycle-test.https.any.html?vp9_p2
  • ✅ full-cycle-test.https.any.worker.html?vp9_p2
  • reconfiguring-encoder.https.any.html?h264_annexb
  • reconfiguring-encoder.https.any.html?h264_avc
  • ✅ reconfiguring-encoder.https.any.html?vp9_p2
  • reconfiguring-encoder.https.any.worker.html?h264_annexb
  • reconfiguring-encoder.https.any.worker.html?h264_avc
  • ✅ reconfiguring-encoder.https.any.worker.html?vp9_p2
  • temporal-svc-encoding.https.any.html?h264
  • temporal-svc-encoding.https.any.worker.html?h264

Rationale

Support for different video codecs is out of scope for WebCodecs.

@nt1m nt1m added the test-change-proposal Proposal to add or remove tests for an interop area label Jul 7, 2023
@nt1m
Copy link
Member Author

nt1m commented Jul 7, 2023

@nairnandu @jgraham Can you find someone on the Blink / Gecko side to review? Thanks!

@jgraham
Copy link
Contributor

jgraham commented Jul 7, 2023

@ChunMinChang does this make sense from Gecko's point of view? (I think if the spec doesn't mandate specific codec support we can't really have tests that depend on specific codecs in Interop without significant extra buy-in/work).

@ChunMinChang
Copy link

ChunMinChang commented Jul 7, 2023

WebCodecs doesn't ask the user-agent to support specific codecs. Instead, it has an API that allows users to check if one codec is supported. An idiom way for WPT to run a codec test is to call isConfigSupported first to determine if the test should continue.

Removing av1 or not won't violate the spec, but what's the reason to remove av1? Can't using isConfigSupported first solve this issue?

@nairnandu
Copy link
Contributor

@dalecurtis could you review this proposal for Chromium?

@nt1m
Copy link
Member Author

nt1m commented Jul 7, 2023

I would be ok with updating tests to pass if isConfigSupported is false. Though I don't think codec support reflects the quality of the WebCodecs implementations.

@dalecurtis
Copy link

We wrote the tests to check isConfigSupported with assert_implements_optional, but may have missed some. As @ChunMinChang notes, tests should bail if they detect the UA doesn't support the codec.

@nt1m
Copy link
Member Author

nt1m commented Jul 8, 2023

Seems like those tests indeed use assert_implements_optional, unfortunately, that reports a status of PRECONDITION_FAILED, and for scoring purposes, that counts the test as failing.

I guess this is just #319 then.

@nt1m
Copy link
Member Author

nt1m commented Jul 8, 2023

OK I submitted a PR to fix assert_implements_optional being counted as failing: web-platform-tests/results-analysis#178

cc @DanielRyanSmith

@nt1m nt1m changed the title Remove WebCodecs tests for AV1 codec Remove WebCodecs tests with PRECONDITION_FAILED Aug 10, 2023
@chrishtr
Copy link
Contributor

@nt1m can this issue be closed now given your previous comment?

@nt1m
Copy link
Member Author

nt1m commented Aug 18, 2023

AV1 tests are now passing, but we still have a small subset of tests that have PRECONDITION_FAILED for specific codecs.

@nt1m
Copy link
Member Author

nt1m commented Aug 29, 2023

@chrishtr @jgraham Here's a list of tests we'd like to remove:

  • full-cycle-test.https.any.html?vp9_p2
  • full-cycle-test.https.any.worker.html?vp9_p2
  • reconfiguring-encoder.https.any.html?h264_annexb
  • reconfiguring-encoder.https.any.html?h264_avc
  • reconfiguring-encoder.https.any.html?vp9_p2
  • reconfiguring-encoder.https.any.worker.html?h264_annexb
  • reconfiguring-encoder.https.any.worker.html?h264_avc
  • reconfiguring-encoder.https.any.worker.html?vp9_p2
  • temporal-svc-encoding.https.any.html?h264
  • temporal-svc-encoding.https.any.worker.html?h264

Basically anything that is green and 0 of 1 here: https://wpt.fyi/results/webcodecs?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&view=interop&q=label%3Ainterop-2023-webcodecs , since it is counted as failing for the purposes of scoring, despite the tests explicitly having optional precondition checks for codec support. The alternative would be PRECONDITION_FAILED tests to not be counted towards the number of total tests when scoring, though it doesn't seem like it is easy to do technically (nor is there consensus on changing scoring in general).

Can we get consensus to remove these specific tests? Thanks!

@dalecurtis
Copy link

vp9_2 makes sense, but why the h264 ones? I thought Safari supported that?

@nt1m
Copy link
Member Author

nt1m commented Sep 3, 2023

@dalecurtis @youennf mentioned the temporal bits of h264 might not be supported. In any case, the VideoEncoder.isConfigSupported function reflects that in Safari if you check the test results, which is spec compliant behavior if something isn't supported.

@dalecurtis
Copy link

Hmm, I think we have those things working with VideoToolbox in Chrome, so I guess it's more that Safari doesn't want to support them? If we're deciding that we want common support across at least one codec, I think we should keep the H264 one as part of Interop. It's your call ultimately, but I think that would be most in the spirit of Interop.

@nt1m
Copy link
Member Author

nt1m commented Sep 5, 2023

To me, the scope of the WebCodecs focus area is more about API support, more than individual codec support, so as long as the API reports codec support correctly (e.g. correctly returns false when querying for support), I personally think it's OK.

Though I don't mind too much either way for those tests specifically, some work on implementing support for those tests is being done here: WebKit/WebKit#17414

I would love some consensus around at least the VP9_P2 tests though.

@dalecurtis
Copy link

Removing vp9p2 is fine with Chrome.

@youennf
Copy link

youennf commented Sep 6, 2023

we have those things working with VideoToolbox in Chrome

L1T2 maybe, what about L1T3?

@dalecurtis
Copy link

Ah, yeah L1T2 is the most we support in hardware. L1T3 is only via OpenH264. The reconfiguring-encoder test only uses L1T2, but the temporal one uses up to L1T3. Should we instead blow out the temporal one into h264_l1t2, h264_l1t3, etc and drop the L1T3 version then? @Djuffin

@Djuffin
Copy link

Djuffin commented Sep 6, 2023

If handling of assert_implements_optional is fixed then the fact that L1T3 is unsupported shouldn't be a problem.
We check for support at the very top of svc_test

@nt1m
Copy link
Member Author

nt1m commented Sep 7, 2023

Unfortunately, assert_implements_optional counts as failing tests for scoring purposes, I tried to suggest changing the scoring algorithm to not do so, but that seems controversial: web-platform-tests/results-analysis#178

@jgraham
Copy link
Contributor

jgraham commented Sep 7, 2023

From my point of view, if:

  • A feature is optional
  • Some vendors plan not to implement that optional feature

It makes that feature unsuitable for inclusion in Interop. Of course the web-platform-tests should still exist, and if at some later time everyone agrees that they will indeed implement that optional feature after all then it's fine to include those tests in Interop again.

@jgraham
Copy link
Contributor

jgraham commented Sep 7, 2023

So to be clear it's fine to remove these tests from Interop just on the above basis. I believe we still end up with enough tests with other codecs that we are still meaningfully testing the wider webcodecs feature in configurations where we can actually promise interoperability.

@nt1m
Copy link
Member Author

nt1m commented Sep 7, 2023

I'll go ahead and remove the VP9_P2 tests as a first step, given there is some support for both Gecko (the last comment from James) / Blink.

nt1m added a commit to web-platform-tests/wpt-metadata that referenced this issue Sep 7, 2023
@nt1m
Copy link
Member Author

nt1m commented Sep 7, 2023

Done: web-platform-tests/wpt-metadata@443d7aa

I'll leave this issue open for L1T3 and potentially L1T2.

@youennf
Copy link

youennf commented Sep 8, 2023

L1T2 is the most we support in hardware. L1T3 is only via OpenH264.

Right, and L1T2 + latencyMode=quality might not really be supported either.
I am uncertain whether we want it in Interop2023.
In any case, splitting the test and setting latencyMode to realtime may not hurt.

@youennf
Copy link

youennf commented Sep 18, 2023

Given the current limited support of H264 in non realtime mode, and how the tests are currently written, I would propose to move temporal-svc-encoding.https.any.html?h264 temporal-svc-encoding.https.any.worker.html?h264 out of Interop 2023.
The other h264 tests seem fine to me.

I'd welcome a H264 temporal+realtime test, with the caveat it would need to handle dropped frames since that may happen on bots running lots of tests.

Does that sound good?

@Djuffin
Copy link

Djuffin commented Sep 18, 2023

If we allow frames to be dropped, does it mean that a conforming VideoEncoder implementation may never call the output callback in the realtime mode?

@youennf
Copy link

youennf commented Sep 18, 2023

does it mean that a conforming VideoEncoder implementation may never call the output callback in the realtime mode?

Spec says User Agents may drop frames to achieve the target [bitrate](https://w3c.github.io/webcodecs/#dom-videoencoderconfig-bitrate) and/or [framerate](https://w3c.github.io/webcodecs/#dom-videoencoderconfig-framerate).

From this, it seems conformant. I am not sure how we can tighten the spec.
I also do not think we should write tests accordingly.

I would still hope most implementations would do their best to not drop frames so that we can write tests that can run smoothly even in highly loaded bots.

For instance, if we enable H.264 temporal in realtime mode, we should expect that the callback is called at least once and that the corresponding chunk has its svc metadata set.

From the bot results I saw in WebKit, instead of outputting 30 frames per second, the encoder in realtime mode was outputting 5 frames per second (which probably mean no second temporal layer would be generated at all). I am not sure whether delaying the sending frames plus increasing the bitrate plus decreasing the frame would help generating the second temporal layer.

@Djuffin
Copy link

Djuffin commented Sep 19, 2023

Chromium had to stop using kVTCompressionPropertyKey_DataRateLimits in order to avoid frame drops in realtime mode
(kVTVideoEncoderSpecification_EnableLowLatencyRateControl + kVTCompressionPropertyKey_RealTime)

@nt1m
Copy link
Member Author

nt1m commented Oct 9, 2023

@youennf Hi! Can you summarize tests you think should be removed? I recall you mentioned temporal tests should be removed.

@youennf
Copy link

youennf commented Oct 12, 2023

I would propose to move temporal-svc-encoding.https.any.html?h264 temporal-svc-encoding.https.any.worker.html?h264 out of Interop 2023. These tests are covering L1T3, which is not supported by H264 HW encoders on macOS/iOS platforms. The tests would also need to be updated to use realtime latency mode, which probably needs a rewrite to acknowledge for some potential frame drops.

@nt1m
Copy link
Member Author

nt1m commented Oct 12, 2023

@dalecurtis @Djuffin What do you think?

@dalecurtis
Copy link

It'd be better if we can split out L1T3 into its own test per #375 (comment), since there is partner usage of L1T1/L1T2 IIRC

However if it's not something that other UAs can support in time, moving the whole thing out seems fine.

@nt1m
Copy link
Member Author

nt1m commented Oct 12, 2023

Thanks, I've removed the tests from Interop for now. Closing this as there are no more PRECONDITION_FAILED failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus area: Web Codecs (video) test-change-proposal Proposal to add or remove tests for an interop area
Projects
None yet
Development

No branches or pull requests

8 participants