Refer to getBoundingClientRect as a defintion of the actual crop area #32

youennf · 2022-03-23T11:09:54Z

Make CropTarget have an internal slot to keep a reference to its element.
This fixes #20 and might be useful for serialisation steps.

Preview | Diff

…ent Refer to getBoundingClientRect as a defintion of the actual crop area.

eladalon1983 · 2022-03-24T16:24:00Z

Sorry, you can't keep a weak reference to an Element on a CropTarget, as that won't work when the CropTarget is posted to another document.

index.html

jan-ivar

LGTM. We should improve the "bounding client rectangle" reference to be to a concept rather than to an API method as soon as one is available, but I think this is an improvement.

index.html

eladalon1983 · 2022-03-24T19:52:03Z

index.html

-              supported type, permanently associates that element with a {{CropTarget}}. Subsequent
-              calls return the same {{CropTarget}}. This {{CropTarget}} may be used as input to


It seems useful to specify that two CropTargets produced from the same Element would be treated as equal. This ensures that if things only happen when changing crop-targets, then they won't happen if changing between equivalent targets. For example:

const ct_a1 = produceCropTarget(document.getElementById('id_a')); const ct_a2 = produceCropTarget(document.getElementById('id_a')); const ct_b = produceCropTarget(document.getElementById('id_b')); mst.cropTo(ct_a1); mst.cropTo(ct_a2); // No-op. mst.cropTo(ct_b); // Yes-op.

(lost this comment somehow)

"Treated as equal" is an optimization that should be taken based on underlying realities in the cropTo algorithm, orthogonally to any API decision here. Instead, what seems at stake here is merely:

const ct_a1 = produceCropTarget(document.getElementById('id_a')); const ct_a2 = produceCropTarget(document.getElementById('id_a')); console.log(ct_1 === ct_2); // true or false?

I do agree though this can be left for a separate issue.

We no longer need to specify these rules as they are connected to the same element.
By having a weak reference slot, this kind of ambiguities disappear.

I agree "we no longer need" it, if we ever did, but that's different from whether this PR needs it. I don't have a strong opinion so whatever gets this merged faster.

Whether ct_1 === ct_2 is true or false is something we need to specify at least down the road though, and seems possibly tied to #11 (e.g. element.cropTarget would be compatible with true not false).

eladalon1983 · 2022-03-24T19:55:29Z

index.html

-              with <var>element</var>. The user agent MUST return a {{Promise}} <var>P</var>. The
+              When {{MediaDevices/produceCropTarget}} is called on a given <var>element</var>,
+              the user agent [=create a CropTarget|creates a CropTarget=] with <var>element</var> as input.
+              The user agent MUST return a {{Promise}} <var>P</var>. The


nit: FYI I've done s/P/p for all of the doc in response to review feedback.

eladalon1983 · 2022-03-24T19:59:36Z

index.html

              When cloning an {{HTMLElement}} on which {{MediaDevices/produceCropTarget}} was
-              previously called, the clone MUST NOT be associated with any {{CropTarget}}. If
+              previously called, the clone is not associated with any {{CropTarget}}. If
              {{MediaDevices/produceCropTarget}} is later called on the clone, a new {{CropTarget}}
-              MUST be assigned to it.
+              will be assigned to it.


This PR seems to tackle a few more issues than it set out to tackle. Can we discuss these changes separately? I'm not necessarily opposed, mind you.

To me, this seems related to @youennf's change above to produce a new cropTarget every time, which makes this normative prose redundant. We could remove it entirely as well, since a cloned element is clearly a new element, so I don't see why we need to say anything.

Yes, I made it non normative since it is no longer needed. I would be for removing it entirely.

@youennf's change above to produce a new cropTarget every time

I did not understand this change as requiring a new CropTarget each time, but rather as permitting it.

eladalon1983 · 2022-03-24T20:02:55Z

index.html

+              video track=] to the <a href="https://drafts.csswg.org/cssom-view/#dom-element-getboundingclientrect">
+              bounding client rectangle</a> of <var>cropTarget</var>.[[\Element]].
+              Since the track is restricted to the visible viewport of the [=display-surface=],
+              the captured area will be the intersection of the visible viewport and the element bounding client rectangle.
+              Whenever {{BrowserCaptureMediaStreamTrack/cropTo}} is invoked,


I think this the core of the PR, according to the context set by the attached issue. I'd move everything else to separate PRs and discuss those changes separately.

I'd gladly merge this PR if:

We leave just this paragraph in the PR (lines 236 and onwards).

We remove reference to <var>cropTarget</var>.[[\Element]] and replaced it with Let <var>element</var> be the {{HTMLElement}} for which cropTarget was produced. Then...

(Btw, HTMLElement rather than Element, until we settle issue #13. I'm OK with Element personally, but let's wait for @alvestrand to elaborate his thinking on this topic.)

Let's make progress the regular way. I just spent a lot of time commenting on each thread here to make progress.

Goal of this PR is: introduce the slot, use it in one place to define more precisely cropping. We can split the PR in two.
In the interest of time, I would tend to merge this PR, merge the serialisation PR on top of this one reusing the weak reference slot.

Let's make progress the regular way. I just spent a lot of time commenting on each thread here to make progress.

I don't understand this message.

introduce the slot

My concern with the slot is that I am not sure how it maps onto specifying implementation. I am not keen on leaving anything inside the CropTarget that could leak to a malicious application that's managed to take over the render process. So no indication of which Document it's running in, etc. Using an internal slot seems to suggest this.

youennf · 2022-03-25T14:57:50Z

Sorry, you can't keep a weak reference to an Element on a CropTarget, as that won't work when the CropTarget is posted to another document.

It is true you cannot expose the element to JS of the other document.
But the spec can state that a JS object has a reference to an object that lives in another process.
There are already examples of this pattern and this would make the spec much more easy to write and read I believe.
Some examples:

MediaStreamTracks have a source slot which is in a different process once the track is transferred. Ditto for RTCDataChannel.
ServiceWorkerClient is a JS object that has access to a potentially out-of-process environment it represents. See for instance https://w3c.github.io/ServiceWorker/#client-focus for how it is making good use of it.

As of using 'reference identifier', I do not really see what this term brings.
I would be okay if we state that CropTarget keeps a reference to an element, instead of an element directly.The serialisation steps would store the reference to the element in a slot and we would be good.

As of weak reference or not, GC should not be observable there so I do not anticipate this would change any JS visible behavior. But it seems good clarification for implementors that they would not need to hold a strong ref to the element from CropTargets.

eladalon1983 · 2022-03-25T21:39:08Z

I think the following is simplest, as (1) it does not require reading multiple sections and trying to coordinate them, and (2) it does not mention "weak references", which could be misunderstood.

Let <var>element</var> be the Element from which CropTarget was constructed.
Calls to this method instruct the user agent to start/stop cropping a [=self-capture video track=]
to the <a href="https://drafts.csswg.org/cssom-view/#dom-element-getboundingclientrect">
bounding client rectangle</a> of <var>element</var>.
Since the track is restricted to the visible viewport of the [=display-surface=],
the captured area will be the intersection of the visible viewport and the element
bounding client rectangle.

For serialization, I think we could employ a similar approach. Possibly a slot would be useful there, possibly not; but here, I think this is simplest. Wdyt?

jan-ivar · 2022-03-28T13:25:25Z

Let <var>element</var> be the Element from which CropTarget was constructed.

"Let" implies strong reference. Cross-process this messes with GC. We need something to serialize, either in this PR or the other.

It follows that what we serialize must be a "weak reference" (i.e. not strong). Since this will most certainly be an id in any implementation, "identifier" or "key" would also work, but then we should mention the UA must use a weak map to implement the mapping.

youennf · 2022-03-28T13:49:36Z

I did not follow the discussion so I'm a bit lost where you all are.
In any case, the following approach seems the most promising to me and this PR is based doing that.

Introduce a slot storing a weak reference to the element.
When serializing, store the weak reference.
When deserialising, set the weak reference.
Drop the CropTarget. <-> HTMLElement relationship.

I think I kept the HTMLElement requirement (no API change).
I can further tweak the bits that might be ambiguous or that do not have yet consensus.

index.html

Co-authored-by: Jan-Ivar Bruaroey <[email protected]>

eladalon1983 · 2022-03-28T15:28:38Z

Let <var>element</var> be the Element from which CropTarget was constructed.
"Let" implies strong reference. Cross-process this messes with GC. We need something to serialize, either in this PR or the other.

It follows that what we serialize must be a "weak reference" (i.e. not strong). Since this will most certainly be an id in any implementation, "identifier" or "key" would also work, but then we should mention the UA must use a weak map to implement the mapping.

"Let" implies (to me) that it's a local variable that is destroyed when exiting the context - here, this "let" is only valid for the length of the paragraph. No strong reference.

Like "let kid be the grandchild who is visiting today; dispense candy to kid." This does not imply that the grandchild pointed at by kid will be visiting in perpetuity, or that other grandchildren are not coming tomorrow.

Introduce a slot storing a weak reference to the element.

It might be that using weak references is standard procedure and would be well understood by readers. If that is the case, then I do not wish to delay us further. Could you point out an example of a Transferable/Serializable object that stores a weak reference to an Element in the original context, and where the weak reference gets replicated in the new context?

eladalon1983 · 2022-03-28T18:03:12Z

I feel compelled to bring up this comment here as well. I think we can avoid discussion bifurcation if we focus on the CSS-like aspects of the current PR and leave serialization and slots to issue #21 and its associated PRs (#24 now, but possibly with follow-ups).

jan-ivar · 2022-03-29T13:04:21Z

"let" is only valid for the length of the paragraph.

We don't normally use "let" in paragraphs, we use them in algorithms, where it is clear what thread and process we're on. That this paragraph is vague about this, seems problematic. The very next sentence in what you propose says "Calls to this method", which to me implies this is talking about the main thread of the capturer process, where making any reference to the element itself would be a cross-process reference, which seems like a mistake.

eladalon1983 · 2022-03-29T13:12:58Z

I think the cleanest solution is to go back to the UUID approach. Namely, we associate both Element and CropTarget with a unique identifier (e.g. UUID, but not necessarily). We then say that cropTo(cropTarget) produces an effect that's dependent on the Element that's associated with that identifier. It's similar to using a "weak reference" but it's immediately apparent that no other action can be affected that uses or abuses this reference, since it's quick to verify that only cropTo even accepts a CropTarget, and cropTo() is patently safe.

We then leave it up to the implementation whether the associated of Element and unique identifier is via an internal slot[*], a mapping in some process, etc. It's out of scope for us.

--
[*] For my education - possibly an internal slot does not imply memory allocation behind the scenes?

youennf · 2022-03-29T13:31:52Z

Could you point out an example of a Transferable/Serializable object that stores a weak reference to an Element in the original context, and where the weak reference gets replicated in the new context?

MessagePort is pretty close to that model, please have a look.
A MessagePort A has a related MessagePort B to which it sends messages.
When transferring A, a reference to its MessagePort B needs to be as a slot of a DataHolder.
Then when recreating A, the new MessagePort A' will set its MessagePort B from the DataHolder structure.

The spec wording is not using weak reference which I think is fine.
Initially I was not using weak reference here (the GC note was sufficient to me) but adding it makes things a tad clearer I think.
If Chrome is implementing this with UUIDs as the weak reference tool, this is perfectly fine.

alvestrand · 2022-03-31T13:46:21Z

index.html

+              <p>Let <var>cropTarget</var> be a new object of type {{CropTarget}}.</p>
+            </li>
+            <li>
+              <p>Let <var>weakRef</var> be a weak reference to <var>element</var>.</p>


Can you suggest a link that we can use for the term "weak reference"?

Those are two terms: "weak" and "reference".

The precedent here is HTML which uses the word "weak" 12 times without defining it, and uses the word "reference" 176 times.

This PR seems consistent with precedent. The term "weak" is used in TAG design principles related to garbage collection, so it seems clear that a "reference" is implicitly strong (affects GC) unless it is preceeded by the word "weak". cc @annevk

Make CropTarget have an internal slot to keep a reference to its elem…

a6f0b06

…ent Refer to getBoundingClientRect as a defintion of the actual crop area.

youennf force-pushed the introduce-element-as-internal-slot branch from 87c9f32 to a6f0b06 Compare March 23, 2022 11:15

jan-ivar reviewed Mar 24, 2022

View reviewed changes

index.html Outdated Show resolved Hide resolved

jan-ivar approved these changes Mar 24, 2022

View reviewed changes

jan-ivar reviewed Mar 24, 2022

View reviewed changes

index.html Outdated Show resolved Hide resolved

jan-ivar mentioned this pull request Mar 24, 2022

Make CropTarget serializable #24

Closed

eladalon1983 reviewed Mar 24, 2022

View reviewed changes

jan-ivar reviewed Mar 28, 2022

View reviewed changes

index.html Outdated Show resolved Hide resolved

Update index.html

a788c0e

Co-authored-by: Jan-Ivar Bruaroey <[email protected]>

jan-ivar approved these changes Mar 28, 2022

View reviewed changes

alvestrand reviewed Mar 31, 2022

View reviewed changes

jan-ivar approved these changes Mar 31, 2022

View reviewed changes

jan-ivar added the Editors can integrate label Mar 31, 2022

eladalon1983 approved these changes Mar 31, 2022

View reviewed changes

Merge branch 'main' into introduce-element-as-internal-slot

1acb49b

eladalon1983 approved these changes Mar 31, 2022

View reviewed changes

eladalon1983 merged commit a60c34f into w3c:main Mar 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refer to getBoundingClientRect as a defintion of the actual crop area #32

Refer to getBoundingClientRect as a defintion of the actual crop area #32

youennf commented Mar 23, 2022 •

edited by pr-preview bot

Loading

eladalon1983 commented Mar 24, 2022 •

edited

Loading

jan-ivar left a comment

eladalon1983 Mar 24, 2022

jan-ivar Mar 28, 2022

youennf Mar 28, 2022

jan-ivar Mar 28, 2022

eladalon1983 Mar 24, 2022

eladalon1983 Mar 24, 2022

jan-ivar Mar 25, 2022

youennf Mar 28, 2022

eladalon1983 Mar 28, 2022

eladalon1983 Mar 28, 2022

eladalon1983 Mar 24, 2022 •

edited

Loading

jan-ivar Mar 25, 2022

youennf Mar 28, 2022

eladalon1983 Mar 28, 2022 •

edited

Loading

youennf commented Mar 25, 2022

eladalon1983 commented Mar 25, 2022

jan-ivar commented Mar 28, 2022

youennf commented Mar 28, 2022

eladalon1983 commented Mar 28, 2022 •

edited

Loading

eladalon1983 commented Mar 28, 2022

jan-ivar commented Mar 29, 2022

eladalon1983 commented Mar 29, 2022 •

edited

Loading

youennf commented Mar 29, 2022

alvestrand Mar 31, 2022

jan-ivar Mar 31, 2022

		supported type, permanently associates that element with a {{CropTarget}}. Subsequent
		calls return the same {{CropTarget}}. This {{CropTarget}} may be used as input to

Refer to getBoundingClientRect as a defintion of the actual crop area #32

Refer to getBoundingClientRect as a defintion of the actual crop area #32

Conversation

youennf commented Mar 23, 2022 • edited by pr-preview bot Loading

eladalon1983 commented Mar 24, 2022 • edited Loading

jan-ivar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladalon1983 Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladalon1983 Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

youennf commented Mar 25, 2022

eladalon1983 commented Mar 25, 2022

jan-ivar commented Mar 28, 2022

youennf commented Mar 28, 2022

eladalon1983 commented Mar 28, 2022 • edited Loading

eladalon1983 commented Mar 28, 2022

jan-ivar commented Mar 29, 2022

eladalon1983 commented Mar 29, 2022 • edited Loading

youennf commented Mar 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youennf commented Mar 23, 2022 •

edited by pr-preview bot

Loading

eladalon1983 commented Mar 24, 2022 •

edited

Loading

eladalon1983 Mar 24, 2022 •

edited

Loading

eladalon1983 Mar 28, 2022 •

edited

Loading

eladalon1983 commented Mar 28, 2022 •

edited

Loading

eladalon1983 commented Mar 29, 2022 •

edited

Loading