Same here - my LGTM still stands with the same reasoning as Yoav. On Tue, May 24, 2022, 2:55 AM 'Elad Alon' via blink-dev < blink-dev@chromium.org> wrote:
> Thank you, Yoav. I'll definitely follow up on those PRs. > > On Tuesday, May 24, 2022 at 11:46:44 AM UTC+2 yoav...@chromium.org wrote: > >> After carefully studying the discussions on the various threads (as well >> as participating in them, trying to bridge the gaps), my previous LGTM >> still stands. >> >> I believe there are 3 points of contention: >> * API shape esthetics >> <https://github.com/w3c/mediacapture-region/issues/11> (#11) >> * Async nature of CropTarget minting >> <https://github.com/w3c/mediacapture-region/issues/17> (#17) >> * Whether CropTarget minting should be able to fail >> <https://github.com/w3c/mediacapture-region/issues/48> (#48) >> >> On API shape esthetics, we've managed to reach a reasonable compromise >> that's being defined in #50 >> <https://github.com/w3c/mediacapture-region/pull/50>. Please make sure >> that we're shipping the shape defined there, as well as that the PR itself >> lands at some point. >> >> As for the need for async minting and their potential failure, I tried to >> clarify the processing model in #47 >> <https://github.com/w3c/mediacapture-region/pull/47>. It'd be great if >> we can land those parts in the spec as well. >> At the same time, the discussions on #17 and #48 don't seem to converge, >> and revolve mostly around back-seat implementation design, which IMO is >> somewhat out-of-place for a WG discussion. >> >> Going with an *async API that can fail* is justified given the >> implementation experience we have so far for this feature, and seems like >> an overall more *conservative future-compat choice*, as we can go back >> on either of those decisions if future implementations prove that either or >> both of those characteristics are not needed. It also seems like these are >> not >> issues that web developers that used the feature as part of its Origin >> Trial deem critical >> <https://github.com/w3c/mediacapture-region/issues/17#issuecomment-1134934556> >> . >> >> >> >> On Thu, May 5, 2022 at 10:09 PM Yoav Weiss <yoav...@chromium.org> wrote: >> >>> Hey Jan-Ivar, >>> >>> Apologies, as I missed the recent activity on issue 11 >>> <https://github.com/w3c/mediacapture-region/issues/11#issuecomment-1112514784> >>> before >>> LGTMing. I'll re-review in that light. >>> >>> On Thu, May 5, 2022 at 9:22 PM Jan-Ivar Bruaroey <jbru...@mozilla.com> >>> wrote: >>> >>>> Hi Youav, the WG is making progress in >>>> https://github.com/w3c/mediacapture-region/issues/11#issuecomment-1112514784 >>>> with good feedback from other Google folks. This progress suggests a >>>> likelihood the WG will go a different direction here, which means Chrome >>>> shipping now would most likely create a web compatibility headache. Can it >>>> be held off a month to resolve this? >>>> >>>> The referenced TAG meeting notes, say it's "*Not the TAG's job to pick >>>> a winner*" which seems to conflict with a LGTM. The WG is making >>>> progress, so it seems premature to expect the TAG to call disputes, which >>>> it sounds like they're saying. Also: >>>> >>>> 1. The statement *"interoperability is an imperative ... not what >>>> is most technically pure"* seems out of place wrt Origin trials. If >>>> interoperability with an Origin Trial is now a thing, it's an argument >>>> to >>>> stop having them. >>>> >>>> I don't think anyone is arguing in favor of interoperability with the >>> existing Origin Trial. >>> >>> >>>> >>>> 1. Web APIs are forever. If we can't consider "technical purity" of >>>> a brand new API ahead of shipping, then when? >>>> >>>> 2. The statement *"a lot of elements not visible... these make no >>>> sense to set a crop target on"* — means the current API is >>>> misleading: The "target" construct (which isn't "set") has nothing >>>> inherently to do with cropping, but is merely solving the sub-problem of >>>> passing an element from a different realm into the track.cropTo() >>>> method. >>>> As discussed in #11 better ways would be to reuse element.id or a >>>> new element.weakRef serializable interface. >>>> >>>> Finally, the absence of any argument that the present API is better in >>>> ANY respects for web developers, should cause pause. All arguments I've >>>> heard instead seem centered on what's easier for browsers to implement >>>> (where "easier" subjectively seems tied to choices the Chrome >>>> implementation has made). Better API shapes are on the table, and no-one's >>>> arguing they're unimplementable. >>>> >>>> We should do better for web developers here and get the shape right. >>>> >>>> Cheers, >>>> >>>> On Monday, May 2, 2022 at 7:16:33 AM UTC-4 yoav...@chromium.org wrote: >>>> >>>>> LGTM1 >>>>> >>>>> Thanks for initiating those discussions. My read of the minutes is >>>>> that they consider the async approach to be fine, and don't arbitrate on >>>>> the naming questions, other than saying that none of the proposals seem >>>>> better than where this API has landed. (and some may add confusion) >>>>> As such it seems that going with the API as currently defined doesn't >>>>> bear significant interoperability risk. >>>>> >>>>> >>>>> >>>>> On Mon, May 2, 2022 at 1:03 PM Elad Alon <elad...@google.com> wrote: >>>>> >>>>>> The discussions around Region Capture have been brought up with TAG >>>>>> again (after their original approval >>>>>> <https://github.com/w3ctag/design-reviews/issues/710#issuecomment-1055212077> >>>>>> of the design). Here are the minutes from that second meeting: >>>>>> >>>>>> https://github.com/w3ctag/meetings/blob/gh-pages/2022/telcons/04-25-minutes.md#media-capture-region >>>>>> >>>>>> On Wednesday, April 20, 2022 at 6:33:52 PM UTC+2 yoav...@chromium.org >>>>>> wrote: >>>>>> >>>>>>> Just to (belatedly) update this thread: Following a discussion with >>>>>>> the API owners and the intent owner a few weeks back, they are planning >>>>>>> to >>>>>>> try and get more folks to weigh in on the open issues, and hopefully >>>>>>> break >>>>>>> the tie. >>>>>>> >>>>>>> On Wednesday, March 23, 2022 at 6:28:30 PM UTC+1 Elad Alon wrote: >>>>>>> >>>>>>>> It may be better to ask actual web developers regarding the least >>>>>>>>> confusing option amongst those proposed. >>>>>>>> >>>>>>>> >>>>>>>> The Web-developers I am in contact with are happiest with >>>>>>>> CropTarget. One of them has mentioned that on issue #18 >>>>>>>> <https://github.com/w3c/mediacapture-region/issues/18>. >>>>>>>> Other Web-developers have not shown up with a preference one way or >>>>>>>> another. >>>>>>>> >>>>>>>> It bears mentioning that we have been discussing the API in the >>>>>>>> WebRTC Working Group for approximately 14 >>>>>>>> <https://docs.google.com/presentation/d/1crumgYj4eHkjo04faLktPTg0QoYJhTFoosEBudfJBuw/edit#slide=id.g7954c29f8a_2_0> >>>>>>>> months >>>>>>>> <https://docs.google.com/presentation/d/1crumgYj4eHkjo04faLktPTg0QoYJhTFoosEBudfJBuw/edit#slide=id.g7954c29f8a_2_0>. >>>>>>>> The initial name for this part of the API was CropID. It was >>>>>>>> changed >>>>>>>> <https://github.com/w3c/mediacapture-region/commit/a60b62cb8946d2c6f79de57ff54bb8cd2a0b8550> >>>>>>>> to >>>>>>>> CropTarget ~4 months ago, following discussions in the WG. Youenn >>>>>>>> filed issue >>>>>>>> #18 <https://github.com/w3c/mediacapture-region/issues/18> ~2 >>>>>>>> months ago. During those two months, no WG member, browser-implementer >>>>>>>> or >>>>>>>> Web-developer voiced concerns about the "CropTarget" name. Youenn has >>>>>>>> made >>>>>>>> several suggestions (Viewport, LayoutBox). I believe I have addressed >>>>>>>> these >>>>>>>> suggestions. I do not think there is interest in the WG for changing >>>>>>>> the >>>>>>>> name. I think the name CropTarget will end up sticking, and not >>>>>>>> produce a >>>>>>>> compat risk. >>>>>>>> >>>>>>>> Sync vs. async cropTarget creation seems like something you'd want >>>>>>>>> to decide on before shipping. >>>>>>>> >>>>>>>> >>>>>>>> It is something we have tried reaching consensus on. But I am not >>>>>>>> observing convergence. I proposed the following: >>>>>>>> >>>>>>>> - For Chrome, it is important to use a Promise<CropTarget>. >>>>>>>> - For any browser that does not feel a Promise is necessary, >>>>>>>> they can immediately return a pre-resolved Promise<CropTarget>. >>>>>>>> - Web-developers would be virtually unaffected by the addition >>>>>>>> of a Promise even - for the sake of argument - if it isn't strictly >>>>>>>> necessary. (I still think it is necessary.) >>>>>>>> >>>>>>>> You mentioned on the thread that the browser can refuse to mint new >>>>>>>>> cropTargets in some cases. What happens then? Is it specified? How are >>>>>>>>> developers supposed to defensively program their API use against that? >>>>>>>> >>>>>>>> >>>>>>>> Failure to mint additional tokens happens if excessive tokens are >>>>>>>> minted. (Defends against memory-overuse in the browser process.) >>>>>>>> Failure is reflected by a Promise being rejected rather than >>>>>>>> fulfilled - which is an established pattern and well-understood by >>>>>>>> Web-developers. >>>>>>>> >>>>>>>> >>>>>>>> If minting couldn't fail, then (naively) writing the >>>>>>>>> process/origin<->token mapping in the browser process could've been >>>>>>>>> done >>>>>>>>> async, while the creation of the token could be sync. >>>>>>>> >>>>>>>> >>>>>>>> That is an interesting alternative; thank you for suggesting it. I >>>>>>>> have given it thought, and I see some issues with it. To start with, an >>>>>>>> application could be given a "dead" token. Such a token will never be >>>>>>>> useful, but the application would not be able detect that until it >>>>>>>> calls >>>>>>>> cropTo(token), and that call fails. Then, this failure would only be >>>>>>>> detected by inspecting the error returned by cropTo(). But note that >>>>>>>> often, >>>>>>>> produceCropTarget() and cropTo() are called by different documents, so >>>>>>>> now >>>>>>>> we even need to postMessage() back a message that "your call to >>>>>>>> produceCropTarget didn't really work, you have a dead token." >>>>>>>> >>>>>>>> So, if minting itself fails, that's preferable in two ways: >>>>>>>> >>>>>>>> 1. The failure is recognized closer to the actual point of >>>>>>>> failure. (And detected by the concerned entity.) >>>>>>>> 2. The application might even wish to *forego calling >>>>>>>> getDisplayMedia()* if it knows it's got a bad token (or rather >>>>>>>> - no token). >>>>>>>> 3. The application is *not* left holding a "dead" token. >>>>>>>> Instead, it holds a rejected Promise<CropTarget> - an established >>>>>>>> pattern >>>>>>>> for failed async-construction. >>>>>>>> 4. If the conditions that caused minting to fail change, then >>>>>>>> it's clear that calling produceCropTarget() again might work. >>>>>>>> (Whereas a >>>>>>>> dead token raises a question - is it *now* OK to use, or should we >>>>>>>> produce >>>>>>>> a new non-dead token?) >>>>>>>> >>>>>>>> Does that make sense? >>>>>>>> >>>>>>>> This seems mostly like a developer ergonomics question. As such, >>>>>>>>> (and like above) a signal from web developers could go a long way to >>>>>>>>> break >>>>>>>>> the tie. >>>>>>>> >>>>>>>> >>>>>>>> One of my partners from Google Slides has commented >>>>>>>> <https://github.com/w3c/mediacapture-region/issues/11#issuecomment-1076543965> >>>>>>>> as >>>>>>>> much. She prefers the approach we took - >>>>>>>> MediaDevices.produceCropTarget. >>>>>>>> (No Web-developers have mentioned a preference for the other approach - >>>>>>>> Element.produceCropTarget.) >>>>>>>> >>>>>>>> On Wed, Mar 23, 2022 at 7:01 AM Yoav Weiss <yoav...@chromium.org> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Monday, March 21, 2022 at 9:15:21 PM UTC+1 Elad Alon wrote: >>>>>>>>> >>>>>>>>>> P.S: Requesting to ship gaplessly. >>>>>>>>>> >>>>>>>>>> On Monday, March 21, 2022 at 9:13:30 PM UTC+1 Elad Alon wrote: >>>>>>>>>> >>>>>>>>>>> Contact emailselad...@chromium.org, mfo...@chromium.org, >>>>>>>>>>> jop...@chromium.org >>>>>>>>>>> >>>>>>>>>>> Explainer >>>>>>>>>>> https://github.com/w3c/mediacapture-region/blob/main/README.md >>>>>>>>>>> >>>>>>>>>>> Specificationhttps://w3c.github.io/mediacapture-region/ >>>>>>>>>>> >>>>>>>>>>> Summary >>>>>>>>>>> >>>>>>>>>>> We introduce a performant and robust API for cropping a >>>>>>>>>>> self-capture video track. (Recall that applications may *already* >>>>>>>>>>> video-capture the tab in which the application is run using >>>>>>>>>>> getDisplayMedia(). Using our new Region Capture, such an >>>>>>>>>>> application may >>>>>>>>>>> now *crop* that track and remove some content from it; typically >>>>>>>>>>> before >>>>>>>>>>> sharing it remotely.) >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Blink componentBlink >>>>>>>>>>> <https://bugs.chromium.org/p/chromium/issues/list?q=component:Blink> >>>>>>>>>>> >>>>>>>>>>> TAG reviewhttps://github.com/w3ctag/design-reviews/issues/710 >>>>>>>>>>> >>>>>>>>>>> TAG review statusNot applicable >>>>>>>>>>> TAG was positive: "Thank you for bringing this to our attention, >>>>>>>>>>> and we are happy to see this proposal move forward." >>>>>>>>>>> They did suggest a change of name (Region Capture -> Tab Region >>>>>>>>>>> Capture), but that does not affect the API. This proposal to refine >>>>>>>>>>> the >>>>>>>>>>> name will be brought up with the WG. >>>>>>>>>>> >>>>>>>>>>> Risks >>>>>>>>>>> >>>>>>>>>>> Interoperability and Compatibility >>>>>>>>>>> >>>>>>>>>>> Remaining open issues with Mozilla and Apple: >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> Thanks for summing up the open issues! :) >>>>>>>>> >>>>>>>>>> >>>>>>>>>>> - The name "CropTarget" - see >>>>>>>>>>> https://github.com/w3c/mediacapture-region/issues/18. No >>>>>>>>>>> alternative has yet been presented which garnered more support >>>>>>>>>>> than >>>>>>>>>>> "CropTarget". This seems unlikely to change. >>>>>>>>>>> >>>>>>>>>>> It may be better to ask actual web developers regarding the >>>>>>>>> least confusing option amongst those proposed. >>>>>>>>> In the past I've used Twitter polls and developer surveys for that >>>>>>>>> purpose. Is this something you considered? >>>>>>>>> Maybe devrel folks can help on that front. >>>>>>>>> >>>>>>>>>> >>>>>>>>>>> - Whether produceCropTarget should return a >>>>>>>>>>> Promise<CropTarget> or a CropTarget - see >>>>>>>>>>> https://github.com/w3c/mediacapture-region/issues/17. In >>>>>>>>>>> internal discussions we have consensus that returning a Promise >>>>>>>>>>> is >>>>>>>>>>> preferrable. However, if the WG settles on returning a >>>>>>>>>>> CropTarget directly, >>>>>>>>>>> a migration plan would be needed to ensure Web applications are >>>>>>>>>>> not broken. >>>>>>>>>>> This would be easier if such a change is either not made at all, >>>>>>>>>>> or is made >>>>>>>>>>> in concert with the next bullet-point. >>>>>>>>>>> >>>>>>>>>>> Sync vs. async cropTarget creation seems like something you'd >>>>>>>>> want to decide on before shipping. You mentioned on the thread that >>>>>>>>> the >>>>>>>>> browser can refuse to mint new cropTargets in some cases. What happens >>>>>>>>> then? Is it specified? How are developers supposed to defensively >>>>>>>>> program >>>>>>>>> their API use against that? >>>>>>>>> If minting couldn't fail, then (naively) writing the >>>>>>>>> process/origin<->token mapping in the browser process could've been >>>>>>>>> done >>>>>>>>> async, while the creation of the token could be sync. >>>>>>>>> >>>>>>>>> >>>>>>>>>>> - API surface of produceCropTarget - see >>>>>>>>>>> https://github.com/w3c/mediacapture-region/issues/11. We >>>>>>>>>>> want MediaDevices.produceCropTarget(), whereas Apple wants >>>>>>>>>>> Element.produceCropTarget or possibly Element.cropTarget(). >>>>>>>>>>> Should the WG >>>>>>>>>>> settle on Apple's current preference, migration would be very >>>>>>>>>>> easy, as we >>>>>>>>>>> can first expose on the new surface *in addition* and then >>>>>>>>>>> deprecate the >>>>>>>>>>> old surface gradually. Moreover, such a migration would actually >>>>>>>>>>> have the >>>>>>>>>>> potential of making a (Promise<CropTarget> -> CropTarget) >>>>>>>>>>> migration >>>>>>>>>>> simpler, should such a change also be adopted by the WG. >>>>>>>>>>> >>>>>>>>>>> This seems mostly like a developer ergonomics question. As such, >>>>>>>>> (and like above) a signal from web developers could go a long way to >>>>>>>>> break >>>>>>>>> the tie. >>>>>>>>> >>>>>>>>>> Other topics under discussion mostly deal with changes to >>>>>>>>>>> spec-language, and will not affect the shipped API. Exception - >>>>>>>>>>> serializability, but that wouldn't break Web-apps (since it's >>>>>>>>>>> mostly opaque >>>>>>>>>>> to the application, which would typically only postMessage the >>>>>>>>>>> CropTarget >>>>>>>>>>> and use it on the other side). >>>>>>>>>>> >>>>>>>>>>> *Gecko:* No signal ( >>>>>>>>>>> https://github.com/mozilla/standards-positions/issues/621) See >>>>>>>>>>> above clarification about remaining open issues under discussion. >>>>>>>>>>> >>>>>>>>>>> *WebKit:* No signal ( >>>>>>>>>>> https://lists.webkit.org/pipermail/webkit-dev/2022-March/032157.html >>>>>>>>>>> ) See above clarification about remaining open issues under >>>>>>>>>>> discussion. >>>>>>>>>>> >>>>>>>>>>> *Web developers:* Strongly positive This work saw strong >>>>>>>>>>> support from Web developers inside of Google (Meet, Docs, Slides). >>>>>>>>>>> >>>>>>>>>>> Other signals: >>>>>>>>>>> >>>>>>>>>>> Ergonomics >>>>>>>>>>> >>>>>>>>>>> N/A >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Activation >>>>>>>>>>> >>>>>>>>>>> Unchallenging to use. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Security >>>>>>>>>>> >>>>>>>>>>> This is a mechanism by which an application purposefully strips >>>>>>>>>>> away information which it already has access to (via pre-existing >>>>>>>>>>> mechanisms such as getDisplayMedia). >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> WebView Application Risks >>>>>>>>>>> >>>>>>>>>>> N/A >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Debuggability >>>>>>>>>>> >>>>>>>>>>> - >>>>>>>>>>> >>>>>>>>>>> Is this feature fully tested by web-platform-tests >>>>>>>>>>> <https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_platform_tests.md> >>>>>>>>>>> ?No >>>>>>>>>>> >>>>>>>>>>> Flag nameRegionCapture >>>>>>>>>>> >>>>>>>>>>> Tracking bug >>>>>>>>>>> https://bugs.chromium.org/p/chromium/issues/detail?id=1247761 >>>>>>>>>>> >>>>>>>>>>> Launch bug >>>>>>>>>>> https://bugs.chromium.org/p/chromium/issues/detail?id=1168076 >>>>>>>>>>> >>>>>>>>>>> Sample linkshttps://w3c.github.io/mediacapture-region/demo/ >>>>>>>>>>> >>>>>>>>>>> Estimated milestones >>>>>>>>>>> OriginTrial desktop last 101 >>>>>>>>>>> OriginTrial desktop first 98 >>>>>>>>>>> >>>>>>>>>>> Link to entry on the Chrome Platform Status >>>>>>>>>>> https://chromestatus.com/feature/5712447794053120 >>>>>>>>>>> >>>>>>>>>>> Links to previous Intent discussionsIntent to prototype: >>>>>>>>>>> https://groups.google.com/a/chromium.org/g/blink-dev/c/dib14W1B0Xc >>>>>>>>>>> Intent to Experiment: >>>>>>>>>>> https://groups.google.com/a/chromium.org/g/blink-dev/c/yFUX0KfuUlo >>>>>>>>>>> Intent to Extend Experiment: >>>>>>>>>>> https://groups.google.com/a/chromium.org/g/blink-dev/c/ZqndGb9e1wM >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> This intent message was generated by Chrome Platform Status >>>>>>>>>>> <https://chromestatus.com/>. >>>>>>>>>>> >>>>>>>>>> -- > You received this message because you are subscribed to the Google Groups > "blink-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to blink-dev+unsubscr...@chromium.org. > To view this discussion on the web visit > https://groups.google.com/a/chromium.org/d/msgid/blink-dev/653356b8-bda8-4ad8-8c4d-215c85bb1cb9n%40chromium.org > <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/653356b8-bda8-4ad8-8c4d-215c85bb1cb9n%40chromium.org?utm_medium=email&utm_source=footer> > . > -- You received this message because you are subscribed to the Google Groups "blink-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscr...@chromium.org. To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAOMQ%2Bw-FwU5o7VTCHBuMzUko9A2rHkbTKDRCjvZr3YKSeyvMYQ%40mail.gmail.com.
