Bug 191926 - [EME][ClearKey] Add layout test for dynamic change of key and video resolution
Summary: [EME][ClearKey] Add layout test for dynamic change of key and video resolution
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: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-23 09:05 PST by Yacine Bandou
Modified: 2018-12-03 03:26 PST (History)
7 users (show)

See Also:


Attachments
Patch (224.89 KB, patch)
2018-11-23 09:36 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (224.07 KB, patch)
2018-11-27 06:01 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (224.84 KB, patch)
2018-12-03 02:13 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yacine Bandou 2018-11-23 09:05:58 PST
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.
Comment 1 Yacine Bandou 2018-11-23 09:36:34 PST
Created attachment 355520 [details]
Patch
Comment 2 Charlie Turner 2018-11-26 01:30:02 PST
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?
Comment 3 Yacine Bandou 2018-11-26 06:03:52 PST
> 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 ?
Comment 4 Yacine Bandou 2018-11-26 06:10:51 PST
(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.
Comment 5 Charlie Turner 2018-11-26 07:15:16 PST
(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.
Comment 6 Yacine Bandou 2018-11-26 08:50:01 PST
> 
> 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.
Comment 7 Yacine Bandou 2018-11-27 06:01:30 PST
Created attachment 355727 [details]
Patch
Comment 8 Charlie Turner 2018-11-27 08:10:01 PST
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?
Comment 9 Yacine Bandou 2018-11-27 08:32:41 PST
(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.
Comment 10 Charlie Turner 2018-11-27 08:45:32 PST
(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!
Comment 11 Xabier Rodríguez Calvar 2018-12-02 22:35:07 PST
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.
Comment 12 Yacine Bandou 2018-12-03 02:08:28 PST
> 
> > 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.
Comment 13 Yacine Bandou 2018-12-03 02:13:52 PST
Created attachment 356369 [details]
Patch
Comment 14 WebKit Commit Bot 2018-12-03 03:25:15 PST
Comment on attachment 356369 [details]
Patch

Clearing flags on attachment: 356369

Committed r238793: <https://trac.webkit.org/changeset/238793>
Comment 15 WebKit Commit Bot 2018-12-03 03:25:17 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2018-12-03 03:26:45 PST
<rdar://problem/46414895>