Bug 191926 – [EME][ClearKey] Add layout test for dynamic change of key and video resolution

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Yacine Bandou]

Tests the ClearKey DRM key change and video resolution during decoding.

It starts by playing a ClearKey video with a resolution of 240p, then, in the third second, 
the resolution of the video will change to 480p with a new ClearKey key.

[Yacine Bandou]

Created attachment 355520 [details]
Patch

[Charlie Turner]

Comment on attachment 355520 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355520&action=review

LGTM with a small question, thank you for the test case!

> LayoutTests/platform/mac/TestExpectations:1323
> +media/encrypted-media/clearKey/clearKey-cenc-video-playback-mse-multikey.html [ Skip ]

Are there open bugs as to why we need to skip this?

[Yacine Bandou]

> LGTM with a small question, thank you for the test case!
> 
> > LayoutTests/platform/mac/TestExpectations:1323
> > +media/encrypted-media/clearKey/clearKey-cenc-video-playback-mse-multikey.html [ Skip ]
> 
> Are there open bugs as to why we need to skip this?

webkit.org/b/181594 media/encrypted-media/clearKey/clearKey-cenc-video-playback-mse.html [ Skip ]

Since this basic test clearKey-cenc-video-playback-mse.html is ignored so this one should be also.

I'll put the same bug. It's ok for you ?

[Yacine Bandou]

(In reply to Yacine Bandou from comment #3)
> 
> webkit.org/b/181594
> media/encrypted-media/clearKey/clearKey-cenc-video-playback-mse.html [ Skip ]
> 
> Since this basic test clearKey-cenc-video-playback-mse.html is ignored so
> this one should be also.
> 
> I'll put the same bug. It's ok for you ?

Oops! this bug is resolved, I think we need more investigation on these tests in mac and gtk.

[Charlie Turner]

(In reply to Yacine Bandou from comment #4)
> (In reply to Yacine Bandou from comment #3)
> > 
> > webkit.org/b/181594
> > media/encrypted-media/clearKey/clearKey-cenc-video-playback-mse.html [ Skip ]
> > 
> > Since this basic test clearKey-cenc-video-playback-mse.html is ignored so
> > this one should be also.
> > 
> > I'll put the same bug. It's ok for you ?
> 
> Oops! this bug is resolved, I think we need more investigation on these
> tests in mac and gtk.

We should be able to just skip for mac for now, but lets run it through GTK and/or WPE and see what happens. If the failure is not yet in bugzilla, please may you create a new bug and link it to the [ Skip ] here? GTK & WPE should have the same behaviour with EME, but our test expectations have drifted unfortunately.

[Yacine Bandou]

> 
> We should be able to just skip for mac for now, but lets run it through GTK
> and/or WPE and see what happens. If the failure is not yet in bugzilla,
> please may you create a new bug and link it to the [ Skip ] here? GTK & WPE
> should have the same behaviour with EME, but our test expectations have
> drifted unfortunately.

For WPE, all clearKey tests succeed.
For GTK, I check if everything is fine and I will update the gtk test expectations file.

[Yacine Bandou]

Created attachment 355727 [details]
Patch

[Charlie Turner]

Comment on attachment 355727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355727&action=review

> LayoutTests/platform/mac/TestExpectations:1323
> +media/encrypted-media/clearKey/clearKey-cenc-video-playback-mse-multikey.html [ Skip ]

Still missing the bug URL here, does it pass already?

[Yacine Bandou]

(In reply to Charlie Turner from comment #8)
> Comment on attachment 355727 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355727&action=review
> 
> > LayoutTests/platform/mac/TestExpectations:1323
> > +media/encrypted-media/clearKey/clearKey-cenc-video-playback-mse-multikey.html [ Skip ]
> 
> Still missing the bug URL here, does it pass already?

I don't know, I can't test it on mac.

I can create a new bug but what I put in it ?

If I don't skip this test on mac it can cause a regression.

[Charlie Turner]

(In reply to Yacine Bandou from comment #9)
> (In reply to Charlie Turner from comment #8)
> > Comment on attachment 355727 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=355727&action=review
> > 
> > > LayoutTests/platform/mac/TestExpectations:1323
> > > +media/encrypted-media/clearKey/clearKey-cenc-video-playback-mse-multikey.html [ Skip ]
> > 
> > Still missing the bug URL here, does it pass already?
> 
> I don't know, I can't test it on mac.

Ack, sorry I misread the platform directory. That looks OK then. Again, not a formal review, but LGTM!

[Xabier Rodríguez Calvar]

Comment on attachment 355727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355727&action=review

> LayoutTests/media/encrypted-media/clearKey/clearKey-cenc-video-playback-mse-multikey.html:26
> +            else {
> +                if (video.currentTime >= 4) {

This can be an else if

> LayoutTests/media/encrypted-media/clearKey/clearKey-cenc-video-playback-mse-multikey.html:27
> +                if (video.currentTime >= 4) {
> +                    testExpected("video.currentTime", 4, ">=")

No strong opinion on this, just looks a bit weird to me.

[Yacine Bandou]

> 
> > LayoutTests/media/encrypted-media/clearKey/clearKey-cenc-video-playback-mse-multikey.html:27
> > +                if (video.currentTime >= 4) {
> > +                    testExpected("video.currentTime", 4, ">=")
> 
> No strong opinion on this, just looks a bit weird to me.

This is the same technique used in WPT.

[Yacine Bandou]

Created attachment 356369 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 356369 [details]
Patch

Clearing flags on attachment: 356369

Committed r238793: <https://trac.webkit.org/changeset/238793>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46414895>