w3c / media-source

Media Source Extensions
https://w3c.github.io/media-source/
Other
268 stars 58 forks source link

Restore auto-revoking behavior of createObjectURL(MediaSource) to revert breaking change introducing memory leaks #156

Open karlt opened 8 years ago

karlt commented 8 years ago

https://github.com/w3c/media-source/issues/10#issuecomment-151735372 pointed out that removing the auto-revoking behavior would be a breaking change.

https://github.com/w3c/media-source/issues/10#issuecomment-189219426 used two arguments to say this would not be a breaking change:

  1. That clients already needed to revoke explicitly anyway, due to a bug in the File API spec.
  2. That three existing UAs did not implement the auto-revoking behavior.

Firefox does auto-revoke, and so 2 was incorrect. This is tested in https://github.com/w3c/web-platform-tests/commit/7ca2c25204985477514ddb4e9171e7544d98a8e5

I am also not convinced by 1. Whether the precise time of revocation was clear or not, it was clear that the intention was that the client need not revoke.

MSE does not need object URLs of infinite lifetime because these merely provide a mechanism of associating the MediaSource with a media element.

As referenced in https://github.com/w3c/media-source/issues/10#issuecomment-201024017 removing the auto revocation leads to memory leaks. These leaks extend to objects affecting and affected by the MediaSource object.

The second sentence of https://github.com/w3c/media-source/issues/10#issuecomment-208923486 is also incorrect because Firefox, at least, revokes the object URL on the next stable state, and so there is nothing to keep the objects alive.

I don't know the reasons for requiring explicit revocation of File URLs, but I doubt they apply to MSE.

Keeping track of strings for object URLs created, or immediately explicitly revoking, is an unnecessary burden for clients, one that is unexpected in a garbage collected world. This outweighs benefits of a breaking change to be consistent with the File API.

wolenetz commented 8 years ago

While I generally agree with the sentiment that auto-revocation is best for web authors, the time for changing the spec in this way is pretty much past at this point for V1. For VNext, a createFor() method for creating auto-revoking URLs (akin to FILE API method of same semantic) and/or just HTMLMediaElement.srcObject = a MediaSource object would be good to consider. Blink and WebKit have had the non-autorevoking behavior for a long time, and there is a note in the current V1 editor's draft of the MSE spec encouraging web authors to explicitly revoke these URLs.

@jdsmith3000 @mwatson2 Please chime in on triage of this issue.

jdsmith3000 commented 8 years ago

I agree with @wolenetz's comments. It would be extremely difficult to revisit his issue, and adding a createFor() method in VNext seems like the only practical solution now.

karlt commented 8 years ago

For vnext, rather than adding more object URL API, I'd suggest following the example of getUserMedia, removing createObjectURL and requiring srcObject support.

srcObject doesn't yet exist on source elements. I don't know that it would be necessary, but I guess it could be added if required.

I can't comment on how easy it would be to undo damage that slipped in just before the cut-off time.

wolenetz commented 8 years ago

@karlt thank you for your comments. @jdsmith3000 thanks for assisting triage. This sounds unfortunately like VNext at this point. Implementations may want to rapidly incubate some form of createFor() and/or srcObject MSE support (perhaps in WICG) to give web authors less "leaky" ways to attach a MediaSource to an HTMLMediaElement.

wolenetz commented 5 years ago

An update and a proposed route forward for this issue:

Now that we're in vNext and incubating various new features for MSE (like codec switching and exposing the MSE API for use from worker context), this issue is again a priority.

(@jyavenard @jernoble) From discussions this month at FOMS 2019 in NYC, Mozilla folks confirmed they still have the original (pre-MSE-REC) version of auto-revoking MediaSource objectUrls. This is confirmed by inspection of a related MSE web-platform-test: https://wpt.fyi/results/media-source/URL-createObjectURL-revoke.html?label=master&product=chrome%5Bexperimental%5D&product=edge&product=firefox%5Bexperimental%5D&product=safari%5Bexperimental%5D&aligned

Since Firefox is still autorevoking MediaSource object URLs, restoring such behavior as official MSE specified behavior is less likely to be a breaking change.

Problem / Motivation: In Chrome (and likely in other MSE-REC implementations that do not currently auto-revoke MediaSource object URLs) there is an unnecessary burden on web app developers to explicitly revoke their MediaSource object URLs as soon as they are no longer necessary, so as to avoid memory bloat. Further, Chrome currently requires the objectUrl to not be revoked until after the attachment to the Media Element is completed as signaled by "sourceopen", since it does not dereference it until the middle of the resource selection algorithm. Also, the current MSE-in-Workers incubation (#175 - with current prototype Chromium CL: https://chromium-review.googlesource.com/c/chromium/src/+/1405697) uses a MediaSource object URL as the reference postMessage'd by the worker to the main thread, which then uses it to attach the worker-thread-constructed MediaSource object. This means that an immediately enqueued revocation task in the worker context's createObjectUrl(mediaSource) would race the main thread's attachment scheduling.

Proposed solution: For MediaSource object URLs created on the main thread: upon successful URL.createObjectUrl(mediaSourceObject) execution, a task should be queued to revoke the created object URL. Alternatively, we could change this revocation's timing to occur upon successful attachment by a media element to the MediaSource object (as seen below, this would more align with the behavior needed to support MSE-in-Workers attachment using object URLs). For MediaSource object URLs created in a worker context (#175): upon successful attachment by the main thread's media element to the MediaSource object, the object URL should be revoked.

I intend to begin incubation of this proposed solution soon (in the WICG MSE vNext repo, and in Chromium), because:

  1. the MSE REC API forces ongoing burden on web developers to ensure they explicitly revoke, and
  2. the potential impact on users of memory bloat needs to be fixed due to suspected lack of explicit revocation by web apps running on MSE implementations that comply with current MSE REC spec.

Feedback on this proposed solution would be very much appreciated.

Update Sep 19, 2019: I plan to experiment in Chrome soon with one of the proposed solutions, above: "Alternatively, we could change this revocation's timing to occur upon successful attachment by a media element to the MediaSource object (as seen below, this would more align with the behavior needed to support MSE-in-Workers attachment using object URLs)."

karlt commented 5 years ago

Thanks for the helpful summary.

Is the MediaSourceHandle from https://github.com/wicg/media-source/blob/mse-in-workers-using-handle/mse-in-workers-using-handle-explainer.md still the intended direction there, or has this been replaced by object url strings?

I'm having trouble imagining the timing of the effect of URL.revokeObjectURL() if the url can concurrently be dereferenced in both a Window and Worker (or multiple Worklers). Is this defined somewhere?

wolenetz commented 5 years ago

@karlt (https://github.com/w3c/media-source/issues/156#issuecomment-474632432):

Is the MediaSourceHandle from https://github.com/wicg/media-source/blob/mse-in-workers-using-handle/mse-in-workers-using-handle-explainer.md still the intended direction there, or has this been replaced by object url strings?

the likely route forward for now with MSE-in-Workers will be to use object URL strings for MSE attachment rather than that MediaSourceHandle object.

I'm having trouble imagining the timing of the effect of URL.revokeObjectURL() if the url can concurrently be dereferenced in both a Window and Worker (or multiple Worklers). Is this defined somewhere?

That is a good general concern. However, MediaSource object URLs are only useful for attachment to HTML media elements, which live only on the window/main thread. There is no way these MediaSource object URLs are dereference-able in any other context, if I understand correctly. The current closest definition to this is in the proposal, above (https://github.com/w3c/media-source/issues/156#issuecomment-474621370).

karlt commented 4 years ago

That doesn't resolve the problem of what happens when URL.revokeObjectURL() is invoked in Worker scope while racing with the dereference in Window scope.

It also doesn't describe how the URL would be auto-revoked when there is no dereference in Window scope.

A MediaSourceHandle approach addresses this by explicitly transferring ownership, instead of magically giving the URL meaning in a different scope.