Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Managed Media Source Extensions API #329

Merged
merged 49 commits into from
Dec 18, 2023
Merged

Add Managed Media Source Extensions API #329

merged 49 commits into from
Dec 18, 2023

Conversation

jyavenard
Copy link
Member

@jyavenard jyavenard commented Oct 23, 2023

As proposed in #320.


Preview | Diff

Co-authored-by: Marcos Cáceres <[email protected]>
@marcoscaceres marcoscaceres changed the title Managed Media Source Extension: spec change Add Managed Media Source Extensions API Oct 24, 2023
Copy link

@dalecurtis dalecurtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defer to editors for approval, but largely looks good to me.

media-source-respec.html Outdated Show resolved Hide resolved
media-source-respec.html Outdated Show resolved Hide resolved
media-source-respec.html Outdated Show resolved Hide resolved
@jyavenard jyavenard self-assigned this Oct 25, 2023
Copy link

@padenot padenot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of editorial comments, but the general concept looks good to me still.

media-source-respec.html Outdated Show resolved Hide resolved
media-source-respec.html Outdated Show resolved Hide resolved
media-source-respec.html Outdated Show resolved Hide resolved
media-source-respec.html Outdated Show resolved Hide resolved
media-source-respec.html Outdated Show resolved Hide resolved
media-source-respec.html Show resolved Hide resolved
media-source-respec.html Outdated Show resolved Hide resolved
media-source-respec.html Show resolved Hide resolved
media-source-respec.html Outdated Show resolved Hide resolved
media-source-respec.html Outdated Show resolved Hide resolved
media-source-respec.html Outdated Show resolved Hide resolved
media-source-respec.html Outdated Show resolved Hide resolved
media-source-respec.html Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Member

marcoscaceres commented Dec 5, 2023

Ok, I think we have addressed all outstanding feedback. Thanks again @wolenetz for the detailed review.

Can you check you are satisfied with the responses and give a tick ✅ if all looks good.

Please feel free to make any further suggesting directly in the pull request.

If we don't hear back in 2 weeks, we will assume everything is good and merge. We can always follow up with issues or more pull requests after we merge.

In the meantime, we will make sure to check the state of the WPTests and make sure we have covered as much we can for the purpose of interop.

media-source-respec.html Outdated Show resolved Hide resolved
@chrisn
Copy link
Member

chrisn commented Dec 6, 2023

I have just one question: does ManagedMediaSource work with MediaSourceHandle, to enable use of ManagedMediaSource in a Worker, and are there test cases for this?

@jyavenard
Copy link
Member Author

I have just one question: does ManagedMediaSource work with MediaSourceHandle, to enable use of ManagedMediaSource in a Worker, and are there test cases for this?

There's nothing specific about ManagedMediaSource when it comes to working in a Worker. All the apply for MediaSource should apply.

webkit doesn't support that feature at present.

@jyavenard jyavenard merged commit 2cb6b9a into main Dec 18, 2023
2 checks passed
github-actions bot added a commit that referenced this pull request Dec 18, 2023
SHA: 2cb6b9a
Reason: push, by jyavenard

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@chrisn chrisn deleted the managed_mse branch February 8, 2024 14:36
@dzianis-dashkevich
Copy link

dzianis-dashkevich commented Oct 25, 2024

@jyavenard

There's nothing specific about ManagedMediaSource when it comes to working in a Worker. All the apply for MediaSource should apply.

It seems like MSE in workers should be available in Safari 18:

https://developer.apple.com/documentation/safari-release-notes/safari-18-release-notes#Media

Added support for MSE in workers. (123052315)

ManagedMediaSource.canConstructInDedicatedWorker returns true, but this code does not work as expected on Desktop and iOS for Safari 18. ManagedMediaSource never fires sourceopen event:

const workerString = `
let ms;
let mms;
if ('MediaSource' in self) {
  ms = new MediaSource();

  ms.onsourceopen = () => console.log('ms-open');
  ms.onsourceended = () => console.log('ms-ended');
  ms.onsourceclose = () => console.log('ms-close');

  postMessage({ type: 'ms-handle', handle: ms.handle }, [ms.handle]);
}

if ('ManagedMediaSource' in self) {
  mms = new ManagedMediaSource();

  mms.onsourceopen = () => console.log('mms-open');
  mms.onsourceended = () => console.log('mms-ended');
  mms.onsourceclose = () => console.log('mms-close');

  postMessage({ type: 'mms-handle', handle: mms.handle }, [mms.handle]);
}
`;

const blob = new Blob([workerString], { type: 'application/javascript' });
const workerUrl = URL.createObjectURL(blob);
const worker = new Worker(workerUrl);

const videoElement1 = document.createElement('video');
const videoElement2 = document.createElement('video');

worker.onmessage = (e) => {
  console.log('received message from worker: ', e.data.type);

  if (e.data.type === 'ms-handle') {
    videoElement1.srcObject = e.data.handle;
  }

  if (e.data.type === 'mms-handle') {
    videoElement2.srcObject = e.data.handle;
  }
};

Here is the output for Safari 18 Desktop:

[Log] received message from worker:  – "ms-handle" (main.js, line 33)
[Log] received message from worker:  – "mms-handle" (main.js, line 33)
[Log] ms-open (8f6b0cc7-ba5b-4825-b632-6a66e24e3083, line 7)

Here is the output for Safari 18 iOS:

[Log] received message from worker:  – "mms-handle" (main.js, line 33)

Do you know if this is expected?

@jyavenard
Copy link
Member Author

This repo isn't for discussing implementation issues and should be restricted to specs talk only.

In WebKit, for MSE and MMS to move from closed to open readyState, you must provide an alternative source that supports AirPlay, or you must explicitly disable remote playback
see https://jyavenard.github.io/htmltests/tests/ManagedMediaSource/bipbop.html

MSE isn't supported on iPhone, only MMS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants