Bug 180430 - [EME] Add layout test for InitData and InitDataType in CENC encrypted event
Summary: [EME] Add layout test for InitData and InitDataType in CENC encrypted event
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: Miguel Gomez
URL:
Keywords: InRadar
Depends on: 180071
Blocks:
  Show dependency treegraph
 
Reported: 2017-12-05 09:57 PST by Yacine Bandou
Modified: 2018-01-08 10:33 PST (History)
13 users (show)

See Also:


Attachments
Patch (9.77 KB, patch)
2017-12-05 10:53 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (2.24 MB, application/zip)
2017-12-05 11:46 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (2.60 MB, application/zip)
2017-12-05 12:46 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (2.93 MB, application/zip)
2017-12-05 15:41 PST, Build Bot
no flags Details
Patch (11.37 KB, patch)
2017-12-08 18:42 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (75.85 KB, patch)
2017-12-11 10:32 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (84.97 KB, patch)
2017-12-13 10:29 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (2.34 MB, application/zip)
2017-12-13 11:15 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (2.92 MB, application/zip)
2017-12-13 11:59 PST, Build Bot
no flags Details
Patch (85.90 KB, patch)
2017-12-13 15:30 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (85.88 KB, patch)
2017-12-14 02:03 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (88.22 KB, patch)
2017-12-14 04:40 PST, Miguel Gomez
no flags Details | Formatted Diff | Diff
Patch (83.17 KB, patch)
2017-12-23 14:09 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (2.62 MB, application/zip)
2017-12-23 15:19 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yacine Bandou 2017-12-05 09:57:40 PST
This layout test, tests the receipt of the encrypted event with a right initData and InitDataType fields in CENC protection scheme.
Comment 1 Yacine Bandou 2017-12-05 10:53:26 PST
Created attachment 328467 [details]
Patch
Comment 2 Build Bot 2017-12-05 11:46:47 PST
Comment on attachment 328467 [details]
Patch

Attachment 328467 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5503704

New failing tests:
media/encrypted-media/clearKey/clearKey-encrypted-cenc-event.html
Comment 3 Build Bot 2017-12-05 11:46:48 PST
Created attachment 328479 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 4 Build Bot 2017-12-05 12:46:02 PST
Comment on attachment 328467 [details]
Patch

Attachment 328467 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5504275

New failing tests:
media/encrypted-media/clearKey/clearKey-encrypted-cenc-event.html
Comment 5 Build Bot 2017-12-05 12:46:04 PST
Created attachment 328486 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 6 Build Bot 2017-12-05 15:41:40 PST
Comment on attachment 328467 [details]
Patch

Attachment 328467 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5506637

New failing tests:
media/encrypted-media/clearKey/clearKey-encrypted-cenc-event.html
Comment 7 Build Bot 2017-12-05 15:41:41 PST
Created attachment 328510 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 8 Yacine Bandou 2017-12-08 08:02:34 PST
This layout test fails in mac-elcapitan platforms because we are not receiving the encrypted event from EME implementation.

Should I add in TestExpectations of these platforms that the test is expected to fail?
Comment 9 Jer Noble 2017-12-08 10:02:14 PST
(In reply to Yacine Bandou from comment #8)
> This layout test fails in mac-elcapitan platforms because we are not
> receiving the encrypted event from EME implementation.
> 
> Should I add in TestExpectations of these platforms that the test is
> expected to fail?

Yes, go ahead and add a TestExpectation for Sierra and El Capitan; there should be a few entries there already.
Comment 10 Yacine Bandou 2017-12-08 18:42:58 PST
Created attachment 328897 [details]
Patch
Comment 11 Xabier Rodríguez Calvar 2017-12-11 04:06:32 PST
Comment on attachment 328897 [details]
Patch

As discussed privately I see two issues here. First is that we should use current MSE test architecture instead of creating a new one. Second is that for this case MSE is not needed so we can save time by using the regular player instead of the MSE one.
Comment 12 Jer Noble 2017-12-11 09:00:03 PST
(In reply to Xabier Rodríguez Calvar from comment #11)
> Comment on attachment 328897 [details]
> Patch
> 
> As discussed privately I see two issues here. First is that we should use
> current MSE test architecture instead of creating a new one. Second is that
> for this case MSE is not needed so we can save time by using the regular
> player instead of the MSE one.

Okay, the unfortunate part is that the Mac port will never be able to support EME through the standard flat-file, non-HLS, non-MSE path. So please just make a global platform/mac/TestExpectations entry (rather than an El Capitan one) for this test.
Comment 13 Yacine Bandou 2017-12-11 10:32:06 PST
Created attachment 329000 [details]
Patch
Comment 14 Xabier Rodríguez Calvar 2017-12-12 02:24:28 PST
Comment on attachment 329000 [details]
Patch

(In reply to Jer Noble from comment #12)
> Okay, the unfortunate part is that the Mac port will never be able to
> support EME through the standard flat-file, non-HLS, non-MSE path. So please
> just make a global platform/mac/TestExpectations entry (rather than an El
> Capitan one) for this test.

Ok, there is no problem in running it though MSE if it's better for you too, I think we can go with the second option of patch with some small changes:
* place the file under media-source instead of encrypted-media and call it media-source-loader-simple.js
* Rename the objects to MediaSourceLoaderSimple and SourceBufferLoaderSimple.
* In the tests changelog remove the JS functions cause they are not interesting and be a bit more verbose about the files you create. It would be nice to know what medias-enc.js, the media itself and the simple loader includes.
Comment 15 Jer Noble 2017-12-12 11:33:29 PST
(In reply to Xabier Rodríguez Calvar from comment #14)
> Comment on attachment 329000 [details]
> Patch
> 
> (In reply to Jer Noble from comment #12)
> > Okay, the unfortunate part is that the Mac port will never be able to
> > support EME through the standard flat-file, non-HLS, non-MSE path. So please
> > just make a global platform/mac/TestExpectations entry (rather than an El
> > Capitan one) for this test.
> 
> Ok, there is no problem in running it though MSE if it's better for you too,
> I think we can go with the second option of patch with some small changes:
> * place the file under media-source instead of encrypted-media and call it
> media-source-loader-simple.js
> * Rename the objects to MediaSourceLoaderSimple and SourceBufferLoaderSimple.
> * In the tests changelog remove the JS functions cause they are not
> interesting and be a bit more verbose about the files you create. It would
> be nice to know what medias-enc.js, the media itself and the simple loader
> includes.

I don't see a problem with having two tests, one for the simple loading path, and one for the MSE one. We'd just have to skip/fail the simple loading path for Mac. But I'm sure a simple path test would have value for GTK+.
Comment 16 Yacine Bandou 2017-12-13 10:29:52 PST
Created attachment 329230 [details]
Patch
Comment 17 Yacine Bandou 2017-12-13 10:34:11 PST
This patch above adds two tests, one for normal playback and the other for MSE playback, as suggested by Jer.
Comment 18 Build Bot 2017-12-13 11:15:13 PST
Comment on attachment 329230 [details]
Patch

Attachment 329230 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5647412

New failing tests:
media/encrypted-media/clearKey/clearKey-encrypted-cenc-event-mse.html
Comment 19 Build Bot 2017-12-13 11:15:14 PST
Created attachment 329236 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 20 Build Bot 2017-12-13 11:59:49 PST
Comment on attachment 329230 [details]
Patch

Attachment 329230 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5647711

New failing tests:
media/encrypted-media/clearKey/clearKey-encrypted-cenc-event-mse.html
Comment 21 Build Bot 2017-12-13 11:59:50 PST
Created attachment 329241 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 22 Yacine Bandou 2017-12-13 15:30:02 PST
Created attachment 329269 [details]
Patch
Comment 23 Xabier Rodríguez Calvar 2017-12-13 23:32:38 PST
Comment on attachment 329269 [details]
Patch

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

> LayoutTests/platform/mac-elcapitan/TestExpectations:11
> +media/encrypted-media/clearKey/clearKey-encrypted-cenc-event-mse.html [ Skip ]

Please, make this [ Failure ] as they are supposed to work some day.

> LayoutTests/platform/mac-wk2/TestExpectations:520
> +media/encrypted-media/clearKey/clearKey-encrypted-cenc-event-mse.html [ Skip ]

Please, make this [ Failure ] as they are supposed to work some day.
Comment 24 Yacine Bandou 2017-12-14 02:03:48 PST
Created attachment 329337 [details]
Patch
Comment 25 WebKit Commit Bot 2017-12-14 03:43:14 PST
Comment on attachment 329337 [details]
Patch

Clearing flags on attachment: 329337

Committed r225899: <https://trac.webkit.org/changeset/225899>
Comment 26 WebKit Commit Bot 2017-12-14 03:43:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Radar WebKit Bug Importer 2017-12-14 03:44:35 PST
<rdar://problem/36045367>
Comment 28 Miguel Gomez 2017-12-14 04:40:32 PST
Reopening to attach new patch.
Comment 29 Miguel Gomez 2017-12-14 04:40:35 PST
Created attachment 329345 [details]
Patch
Comment 30 Miguel Gomez 2017-12-14 04:42:19 PST
Comment on attachment 329345 [details]
Patch

Sorry, not sure how this patch ended in this bug!!
Comment 31 Xabier Rodríguez Calvar 2017-12-14 05:29:21 PST
(In reply to WebKit Commit Bot from comment #26)
> All reviewed patches have been landed.  Closing bug.
Comment 32 Matt Lewis 2017-12-14 16:03:08 PST
The test media/encrypted-media/clearKey/clearKey-encrypted-cenc-event-mse.html is timing out on multiple platforms and failing on others:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=media%2Fencrypted-media%2FclearKey%2FclearKey-encrypted-cenc-event-mse.html


Judging from the comments the expectations for this rather than putting the test in the  mac-elcapitan/TestExpectations, we should just mark specific OSs within the top level Mac expectations.

EX:

webkit.org/b/180071 [ ElCapitan ] media/encrypted-media/clearKey/clearKey-encrypted-cenc-event-mse.html [ Failure ]

Also it helps to have the bug for every expectation file in case they get changed for some reason
Due to the fact that the test is timing out on multiple Mac platforms, rather than marking it as failing, I'm marking it as skip:

https://trac.webkit.org/changeset/225942/webkit
Comment 33 Michael Catanzaro 2017-12-22 14:28:19 PST
This test is timing out on GTK since it was added in r225899, not failing. Additionally, it looks like Matt's comments above were not addressed. I'm going to roll this out. Please don't intentionally add timeouts: skip the test if need be.
Comment 34 Michael Catanzaro 2017-12-22 14:34:22 PST
Committed r226278: <https://trac.webkit.org/changeset/226278>
Comment 35 Michael Catanzaro 2017-12-22 14:34:56 PST
Reopening.
Comment 36 Michael Catanzaro 2017-12-22 14:36:09 PST
(In reply to Michael Catanzaro from comment #33)
> This test is timing out on GTK since it was added in r225899, not failing.
> Additionally, it looks like Matt's comments above were not addressed. I'm
> going to roll this out. Please don't intentionally add timeouts: skip the
> test if need be.

Should probably be skipped in the global TestExpectations if it's expected to be broken on so many platforms.
Comment 37 Yacine Bandou 2017-12-23 11:40:47 PST
(In reply to Michael Catanzaro from comment #36)
> (In reply to Michael Catanzaro from comment #33)
> > This test is timing out on GTK since it was added in r225899, not failing.
> > Additionally, it looks like Matt's comments above were not addressed. I'm
> > going to roll this out. Please don't intentionally add timeouts: skip the
> > test if need be.
> 
> Should probably be skipped in the global TestExpectations if it's expected
> to be broken on so many platforms.

This test is currently correctly passing in the WPE platform since r225642, see bug 180071.

I do not understand why you removed this test instead of update TestExpectations.
Comment 38 Yacine Bandou 2017-12-23 14:09:09 PST
Created attachment 330163 [details]
Patch
Comment 39 Michael Catanzaro 2017-12-23 14:48:14 PST
Hi, I had no clue what the expectations were supposed to be, except that what you had committed wasn't right. As a rule, if your commit introduces a regression or test failure, you can expect it to be rolled out.

Anyway, thanks for updating the patch. Skipping the new tests everywhere except for WPE seems sane.
Comment 40 Yacine Bandou 2017-12-23 15:13:39 PST
(In reply to Michael Catanzaro from comment #39)
> Hi, I had no clue what the expectations were supposed to be, except that
> what you had committed wasn't right. As a rule, if your commit introduces a
> regression or test failure, you can expect it to be rolled out.
> 
Understood, thanks.
Comment 41 Build Bot 2017-12-23 15:19:04 PST
Comment on attachment 330163 [details]
Patch

Attachment 330163 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5813695

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html
Comment 42 Build Bot 2017-12-23 15:19:05 PST
Created attachment 330164 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 43 Yacine Bandou 2017-12-23 15:54:08 PST
(In reply to Build Bot from comment #41)
> Comment on attachment 330163 [details]
> Patch
> 
> Attachment 330163 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.webkit.org/results/5813695
> 
> New failing tests:
> imported/w3c/web-platform-tests/service-workers/service-worker/register-same-
> scope-different-script-url.https.html

I think, this failure already exists, it is independent of my patch.
Comment 44 Michael Catanzaro 2017-12-24 10:39:33 PST
Comment on attachment 330163 [details]
Patch

Yes, the unrelated test is flaky. You can ignore it.
Comment 45 Yacine Bandou 2018-01-08 05:26:43 PST
Who can review and land this patch ?
Comment 46 Xabier Rodríguez Calvar 2018-01-08 07:04:14 PST
Comment on attachment 330163 [details]
Patch

(In reply to Michael Catanzaro from comment #39)
> Hi, I had no clue what the expectations were supposed to be, except that
> what you had committed wasn't right. As a rule, if your commit introduces a
> regression or test failure, you can expect it to be rolled out.
> 
> Anyway, thanks for updating the patch. Skipping the new tests everywhere
> except for WPE seems sane.
Comment 47 WebKit Commit Bot 2018-01-08 10:33:27 PST
Comment on attachment 330163 [details]
Patch

Clearing flags on attachment: 330163

Committed r226518: <https://trac.webkit.org/changeset/226518>
Comment 48 WebKit Commit Bot 2018-01-08 10:33:30 PST
All reviewed patches have been landed.  Closing bug.