Bug 220046 - media-source-webm.html: Handle frame size in HAVE_METADATA
Summary: media-source-webm.html: Handle frame size in HAVE_METADATA
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alicia Boya García
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-20 14:00 PST by Alicia Boya García
Modified: 2021-01-05 16:33 PST (History)
9 users (show)

See Also:


Attachments
Patch (4.77 KB, patch)
2020-12-20 14:20 PST, Alicia Boya García
no flags Details | Formatted Diff | Diff
Follow-up patch to fix a failure on Big Sur (3.57 KB, patch)
2020-12-23 11:13 PST, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alicia Boya García 2020-12-20 14:00:17 PST
The current version of media-source-webm.html assumes that a `resize`
event happens after the first media segment is appended.

This is not necessarily true. In fact, an initialization segment
should cause a transition to HAVE_METADATA, and that per spec implies
the size of the video is known.

In practice, some implementations don't report this until a media
segment has arrived.

Because of the way the current code is written, an implementation
emitting `resize` on HAVE_METADATA would timeout the test. This patch
fixes that, accomodating both cases.
Comment 1 Alicia Boya García 2020-12-20 14:20:11 PST
Created attachment 416588 [details]
Patch
Comment 2 EWS 2020-12-21 04:15:45 PST
Committed r271020: <https://trac.webkit.org/changeset/271020>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416588 [details].
Comment 3 Radar WebKit Bug Importer 2020-12-21 04:16:25 PST
<rdar://problem/72547169>
Comment 4 Peng Liu 2020-12-22 20:57:11 PST
With this patch, the test starts to fail on Big Sur. :-(
Comment 5 Alicia Boya García 2020-12-23 04:31:43 PST
(In reply to Peng Liu from comment #4)
> With this patch, the test starts to fail on Big Sur. :-(

How does it fail? If it's a change in the order of the lines in the output similar to the one included in the attached patch for the GStreamer ports, that is perfectly fine, just needs a rebaseline. Otherwise, it may have exposed some other bug.

I remember seeing before accomodations in tests for a slight bug where a resize event with 0x0 dimensions was emitted, before another one with the actual dimensions was emitted.
Comment 6 Peng Liu 2020-12-23 09:22:33 PST
Yes, it's a change in the order of lines. I have to revert the change in media-source-webm-expected.txt to fix the failure.

Sounds like both orders are correct? I think it is a good idea to revise the test to support both of them.
Comment 7 Peng Liu 2020-12-23 11:13:04 PST
Created attachment 416719 [details]
Follow-up patch to fix a failure on Big Sur
Comment 8 Alicia Boya García 2020-12-23 11:47:27 PST
(In reply to Peng Liu from comment #6)
> Sounds like both orders are correct? I think it is a good idea to revise the
> test to support both of them.

Both orders are in practice acceptable, although the resize happening after the initialization segment without having to wait for the media segment is more correct in accordance with the spec.

I personally favor a rebaseline over changing the test to keep the output the same in both cases since if your browser handles resize events at the right moment, that's a behavior that ideally you want to protect against regressions.
Comment 9 Peng Liu 2020-12-23 11:56:12 PST
(In reply to Alicia Boya García from comment #8)
> 
> Both orders are in practice acceptable, although the resize happening after
> the initialization segment without having to wait for the media segment is
> more correct in accordance with the spec.
> 
> I personally favor a rebaseline over changing the test to keep the output
> the same in both cases since if your browser handles resize events at the
> right moment, that's a behavior that ideally you want to protect against
> regressions.

Hmm, you mean using a different media-source-webm-expected.txt for Big Sur?
Comment 10 Alicia Boya García 2020-12-25 14:32:01 PST
(In reply to Peng Liu from comment #9)
> Hmm, you mean using a different media-source-webm-expected.txt for Big Sur?

Yes.
Comment 11 Peng Liu 2021-01-05 14:40:08 PST
(In reply to Alicia Boya García from comment #10)
> (In reply to Peng Liu from comment #9)
> > Hmm, you mean using a different media-source-webm-expected.txt for Big Sur?
> 
> Yes.

I had a discussion with Jer about that. While it makes sense to fire the "resize" event before the "update" event for the *first* initialization segment, the order is not important for Apple's ports. To avoid the possible test flakiness due to the WebM parser implementation, we would suggest ignoring the event order in this test.

Is the order important for GTK and WPE ports? If yes, maybe we need to add one test for it?
Comment 12 Alicia Boya García 2021-01-05 14:46:47 PST
(In reply to Peng Liu from comment #11)
> (In reply to Alicia Boya García from comment #10)
> > (In reply to Peng Liu from comment #9)
> > > Hmm, you mean using a different media-source-webm-expected.txt for Big Sur?
> > 
> > Yes.
> 
> I had a discussion with Jer about that. While it makes sense to fire the
> "resize" event before the "update" event for the *first* initialization
> segment, the order is not important for Apple's ports. To avoid the possible
> test flakiness due to the WebM parser implementation, we would suggest
> ignoring the event order in this test.
> 
> Is the order important for GTK and WPE ports? If yes, maybe we need to add
> one test for it?

If it's necessary to avoid flakiness in a particular implementation, doing it in a "order doesn't matter" way it's okay. Certainly a separate test could be made for checking the availability of dimensions on HAVE_METADATA and mark that one as flaky or skip in the implementations that don't support that.
Comment 13 EWS 2021-01-05 16:33:42 PST
Committed r271181: <https://trac.webkit.org/changeset/271181>

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