From 5d309d6f31a279218ff15302e8cfa22c95028267 Mon Sep 17 00:00:00 2001 From: Matt Wolenetz Date: Wed, 13 Jul 2022 18:17:14 -0700 Subject: [PATCH] MSE-in-Workers: Switch getHandle() to simpler [SameObject] handle attr Following discussion in the spec PR #306 [1], this change updates the way to obtain a MediaSourceHandle to be via a "handle" attribute on the MediaSource instance that is both readonly and always returns the same object (or throws exception, if for instance, it is attempted to be read from a main-thread-owned MediaSource instance instead of a dedicated- worker-owned MediaSource instance). Also included is the removal of the readyState check when attempting to obtain this handle (since it is now never expected to be changeable; no sequence of distinct handles is ever expected to be obtainable from a worker MediaSource). Also removed is the prevention of retrieving such a handle from an instance more than once. Multiple tests are added or updated to ensure correct behavior. [1] https://github.com/w3c/media-source/pull/306 BUG=878133 Change-Id: Ic07095d6d1dc95b8e6be818027984600aa7ab334 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3750140 Commit-Queue: Matthew Wolenetz Reviewed-by: Will Cassella Cr-Commit-Position: refs/heads/main@{#1024034} --- .../mediasource-worker-detach-element.js | 2 +- .../mediasource-worker-duration.js | 2 +- ...iasource-worker-handle-transfer-to-main.js | 2 +- .../mediasource-worker-handle-transfer.js | 4 +- .../mediasource-worker-handle.html | 11 ++++ .../mediasource-worker-handle.js | 57 +++++++++++++------ .../mediasource-worker-play.js | 34 ++++++++++- 7 files changed, 88 insertions(+), 24 deletions(-) diff --git a/media-source/dedicated-worker/mediasource-worker-detach-element.js b/media-source/dedicated-worker/mediasource-worker-detach-element.js index 3007f6ed983fa4..54b1d815f25299 100644 --- a/media-source/dedicated-worker/mediasource-worker-detach-element.js +++ b/media-source/dedicated-worker/mediasource-worker-detach-element.js @@ -32,7 +32,7 @@ util.mediaSource.addEventListener("sourceopen", () => { err => { postMessage({ subject: messageSubject.ERROR, info: err }) } ); }, { once : true }); -let handle = util.mediaSource.getHandle(); +let handle = util.mediaSource.handle; postMessage({ subject: messageSubject.HANDLE, info: handle }, { transfer: [handle] } ); diff --git a/media-source/dedicated-worker/mediasource-worker-duration.js b/media-source/dedicated-worker/mediasource-worker-duration.js index d868fc4a1fecfd..2a2c7bac0b2793 100644 --- a/media-source/dedicated-worker/mediasource-worker-duration.js +++ b/media-source/dedicated-worker/mediasource-worker-duration.js @@ -180,7 +180,7 @@ function processPhase(isResponseToAck = false) { case testPhase.kInitial: assert(Number.isNaN(util.mediaSource.duration), "Initial unattached MediaSource duration must be NaN, but instead is " + util.mediaSource.duration); phase = testPhase.kAttaching; - let handle = util.mediaSource.getHandle(); + let handle = util.mediaSource.handle; postMessage({ subject: messageSubject.HANDLE, info: handle }, { transfer: [handle] } ); break; diff --git a/media-source/dedicated-worker/mediasource-worker-handle-transfer-to-main.js b/media-source/dedicated-worker/mediasource-worker-handle-transfer-to-main.js index e83ab75c6a9f67..15cccb1a0e3f72 100644 --- a/media-source/dedicated-worker/mediasource-worker-handle-transfer-to-main.js +++ b/media-source/dedicated-worker/mediasource-worker-handle-transfer-to-main.js @@ -5,6 +5,6 @@ importScripts('mediasource-message-util.js'); // harness, and would confuse the test case message parsing there. // Just obtain a MediaSourceHandle and transfer it to creator of our context. -let handle = new MediaSource().getHandle(); +let handle = new MediaSource().handle; postMessage( {subject: messageSubject.HANDLE, info: handle}, {transfer: [handle]}); diff --git a/media-source/dedicated-worker/mediasource-worker-handle-transfer.js b/media-source/dedicated-worker/mediasource-worker-handle-transfer.js index 4c62aeec7cd63b..803da44e23c234 100644 --- a/media-source/dedicated-worker/mediasource-worker-handle-transfer.js +++ b/media-source/dedicated-worker/mediasource-worker-handle-transfer.js @@ -1,7 +1,7 @@ importScripts('/resources/testharness.js'); test(t => { - let handle = new MediaSource().getHandle(); + let handle = new MediaSource().handle; assert_true(handle instanceof MediaSourceHandle); assert_throws_dom('DataCloneError', function() { postMessage(handle); @@ -9,7 +9,7 @@ test(t => { }, 'MediaSourceHandle serialization without transfer must fail, tested in worker'); test(t => { - let handle = new MediaSource().getHandle(); + let handle = new MediaSource().handle; assert_true(handle instanceof MediaSourceHandle); assert_throws_dom('DataCloneError', function() { postMessage(handle, [handle, handle]); diff --git a/media-source/dedicated-worker/mediasource-worker-handle.html b/media-source/dedicated-worker/mediasource-worker-handle.html index b084fb6d5bb9cb..a921acdec9e401 100644 --- a/media-source/dedicated-worker/mediasource-worker-handle.html +++ b/media-source/dedicated-worker/mediasource-worker-handle.html @@ -33,6 +33,17 @@ }); }, "Test main context receipt of postMessage'd MediaSourceHandle from DedicatedWorker MediaSource"); +test(t => { + assert_true(window.hasOwnProperty("MediaSourceHandle"), "window must have MediaSourceHandle visibility"); + + // Note, MSE spec may eventually describe how a main-thread MediaSource can + // attach to an HTMLMediaElement using a MediaSourceHandle. For now, we + // ensure that the implementation of this is not available. + assert_throws_dom('NotSupportedError', function() { + let h = new MediaSource().handle; + }, 'main thread MediaSource instance cannot (yet) create a usable MediaSourceHandle'); +}, "Test main-thread-owned MediaSource instance cannot create a MediaSourceHandle"); + if (MediaSource.hasOwnProperty("canConstructInDedicatedWorker") && MediaSource.canConstructInDedicatedWorker === true) { // If implementation claims support for MSE-in-Workers, then fetch and run // some tests directly in another dedicated worker and get their results diff --git a/media-source/dedicated-worker/mediasource-worker-handle.js b/media-source/dedicated-worker/mediasource-worker-handle.js index 577b1facbc9fcd..d35cb877c2a73b 100644 --- a/media-source/dedicated-worker/mediasource-worker-handle.js +++ b/media-source/dedicated-worker/mediasource-worker-handle.js @@ -13,9 +13,11 @@ test(t => { }, "MediaSource in DedicatedWorker context must have true-valued canConstructInDedicatedWorker if Window context had it"); test(t => { - assert_true("getHandle" in MediaSource.prototype, "dedicated worker MediaSource must have getHandle"); + assert_true( + 'handle' in MediaSource.prototype, + 'dedicated worker MediaSource must have handle in prototype'); assert_true(self.hasOwnProperty("MediaSourceHandle"), "dedicated worker must have MediaSourceHandle visibility"); -}, "MediaSource prototype in DedicatedWorker context must have getHandle, and worker must have MediaSourceHandle"); +}, 'MediaSource prototype in DedicatedWorker context must have \'handle\', and worker must have MediaSourceHandle'); test(t => { const ms = new MediaSource(); @@ -24,22 +26,45 @@ test(t => { test(t => { const ms = new MediaSource(); - const handle = ms.getHandle(); - assert_not_equals(handle, null, "must have a non-null getHandle result"); + const handle = ms.handle; + assert_not_equals(handle, null, 'must have a non-null \'handle\' attribute'); assert_true(handle instanceof MediaSourceHandle, "must be a MediaSourceHandle"); -}, "mediaSource.getHandle() in DedicatedWorker returns a MediaSourceHandle"); +}, 'mediaSource.handle in DedicatedWorker returns a MediaSourceHandle'); test(t => { - const ms = new MediaSource(); - const handle1 = ms.getHandle(); - let handle2 = null; - assert_throws_dom("InvalidStateError", function() - { - handle2 = ms.getHandle(); - }, "getting second handle from MediaSource instance"); - assert_equals(handle2, null, "getting second handle from same MediaSource must have failed"); - assert_not_equals(handle1, null, "must have a non-null result of the first getHandle"); - assert_true(handle1 instanceof MediaSourceHandle, "first getHandle result must be a MediaSourceHandle"); -}, "mediaSource.getHandle() must not succeed more than precisely once for a MediaSource instance"); + const msA = new MediaSource(); + const msB = new MediaSource(); + const handleA1 = msA.handle; + const handleA2 = msA.handle; + const handleA3 = msA['handle']; + const handleB1 = msB.handle; + const handleB2 = msB.handle; + assert_true( + handleA1 === handleA2 && handleB1 === handleB2 && handleA1 != handleB1, + 'SameObject is observed for mediaSource.handle, and different MediaSource instances have different handles'); + assert_true( + handleA1 === handleA3, + 'SameObject is observed even when accessing handle differently'); + assert_true( + handleA1 instanceof MediaSourceHandle && + handleB1 instanceof MediaSourceHandle, + 'handle property returns MediaSourceHandles'); +}, 'mediaSource.handle observes SameObject property correctly'); + +test(t => { + const ms1 = new MediaSource(); + const handle1 = ms1.handle; + const ms2 = new MediaSource(); + const handle2 = ms2.handle; + assert_true( + handle1 !== handle2, + 'distinct MediaSource instances must have distinct handles'); + + // Verify attempt to change value of the handle property does not succeed. + ms1.handle = handle2; + assert_true( + ms1.handle === handle1 && ms2.handle === handle2, + 'MediaSource handle is readonly, so should not have changed'); +}, 'Attempt to set MediaSource handle property should fail to change it, since it is readonly'); done(); diff --git a/media-source/dedicated-worker/mediasource-worker-play.js b/media-source/dedicated-worker/mediasource-worker-play.js index d2f3aa031cd9bc..5c4760bf7b1a4e 100644 --- a/media-source/dedicated-worker/mediasource-worker-play.js +++ b/media-source/dedicated-worker/mediasource-worker-play.js @@ -9,8 +9,37 @@ onmessage = function(evt) { }; let util = new MediaSourceWorkerUtil(); +let handle = util.mediaSource.handle; + +util.mediaSource.addEventListener('sourceopen', () => { + // Immediately re-verify the SameObject property of the handle we transferred. + if (handle !== util.mediaSource.handle) { + postMessage({ + subject: messageSubject.ERROR, + info: 'mediaSource.handle changed from the original value' + }); + } + + // Also verify that transferring the already-transferred handle instance is + // prevented correctly. + try { + postMessage( + { + subject: messageSubject.ERROR, + info: + 'This postMessage should fail: the handle has already been transferred', + extra_info: util.mediaSource.handle + }, + {transfer: [util.mediaSource.handle]}); + } catch (e) { + if (e.name != 'DataCloneError') { + postMessage({ + subject: messageSubject.ERROR, + info: 'Expected handle retransfer exception did not occur' + }); + } + } -util.mediaSource.addEventListener("sourceopen", () => { sourceBuffer = util.mediaSource.addSourceBuffer(util.mediaMetadata.type); sourceBuffer.onerror = (err) => { postMessage({ subject: messageSubject.ERROR, info: err }); @@ -40,7 +69,6 @@ util.mediaSource.addEventListener("sourceopen", () => { }; util.mediaLoadPromise.then(mediaData => { sourceBuffer.appendBuffer(mediaData); }, err => { postMessage({ subject: messageSubject.ERROR, info: err }) }); -}, { once : true }); +}, {once: true}); -let handle = util.mediaSource.getHandle(); postMessage({ subject: messageSubject.HANDLE, info: handle }, { transfer: [handle] });