| Summary: | [EME] Add layout test for InitData and InitDataType in CENC encrypted event | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Yacine Bandou <bandou.yacine> |
| Component: | Tools / Tests | Assignee: | Miguel Gomez <magomez> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | calvaris, commit-queue, ews-watchlist, jer.noble, jlewis3, lforschler, loic.yhuel, magomez, mcatanzaro, olivier.blin, rniwa, webkit-bug-importer, zan |
| Priority: | P2 | Keywords: | InRadar |
| Version: | WebKit Nightly Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | 180071 | ||
| Bug Blocks: | |||
| Attachments: | |||
|
Description
Yacine Bandou
2017-12-05 09:57:40 PST
Created attachment 328467 [details]
Patch
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 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 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 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 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 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
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? (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. Created attachment 328897 [details]
Patch
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.
(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. Created attachment 329000 [details]
Patch
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. (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+. Created attachment 329230 [details]
Patch
This patch above adds two tests, one for normal playback and the other for MSE playback, as suggested by Jer. 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 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 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 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
Created attachment 329269 [details]
Patch
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. Created attachment 329337 [details]
Patch
Comment on attachment 329337 [details] Patch Clearing flags on attachment: 329337 Committed r225899: <https://trac.webkit.org/changeset/225899> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 329345 [details]
Patch
Comment on attachment 329345 [details]
Patch
Sorry, not sure how this patch ended in this bug!!
(In reply to WebKit Commit Bot from comment #26) > All reviewed patches have been landed. Closing bug. 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 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. Committed r226278: <https://trac.webkit.org/changeset/226278> Reopening. (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. (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. Created attachment 330163 [details]
Patch
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. (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 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 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
(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 on attachment 330163 [details]
Patch
Yes, the unrelated test is flaky. You can ignore it.
Who can review and land this patch ? 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 on attachment 330163 [details] Patch Clearing flags on attachment: 330163 Committed r226518: <https://trac.webkit.org/changeset/226518> All reviewed patches have been landed. Closing bug. |