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

Allow cropTo() to fail due to resource limitation #54

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

eladalon1983
Copy link
Member

@eladalon1983 eladalon1983 commented May 31, 2022

@eladalon1983
Copy link
Member Author

@jan-ivar and @youennf, we have previously discussed how allowing cropTo() to fail was uncontroversial (as opposed to token-minting). Let's have the spec reflect that.

index.html Outdated Show resolved Hide resolved
eladalon1983 and others added 2 commits June 2, 2022 18:55
@eladalon1983
Copy link
Member Author

eladalon1983 commented Jun 2, 2022

  1. I've made the suggested changes. PTAL.
  2. During today's Editors' meeting, @jan-ivar and @youennf have been opposed to merging this PR. If you've changed your mind - great, let's merge. Otherwise - could you please explain why? For transparency's sake, I think it would be good to continue this discussion publicly. I know some folks would be interested, e.g. @yoavweiss.

@youennf
Copy link
Contributor

youennf commented Jun 2, 2022

2. During today's Editors' meeting, @jan-ivar and @youennf have been opposed to merging this PR.

Here are some thoughts that have been shared during this editor's call:

  1. This PR needs more work to be merged. I made for instance the comment that the new check should be done asynchronously. @jan-ivar mentioned a different error type would be better.
  2. My position on this particular issue is not set yet, this issue was filed 2 days ago and I was surprised the goal was to merge it today. Pushing back on merging this PR is not a hint on what my position will be.
  3. It does not seem like we need to rush this PR like we did rush the fromElement PR. fromElement has much more consequence than this PR, so it is not great but ok to rush it. Here, it seems ok to take some time. I would personally prefer if we would allocate time on the async/sync discussion instead of those issues.
  4. This PR does not seem like a blocker for shipping Chrome implementation (?)
  5. There are some arguments for/against this PR, it seems worth evaluating them. Let's take the time to discuss these arguments before merging the PR instead of after.

With regards to 4 (the most interesting part of the discussion), some more thoughts, either shared during the call or not:

  1. It seems that most specs are not describing algorithms to that kind of details. Maybe this is the first time we are trying to do so and we try to create a pattern. Or maybe this is an existing pattern that has been discussed and validated in other specs and we are just applying it here. Let's clarify this.
  2. Resource limitation is quite vague in general so it is unclear how this helps interoperability or how this is supposed to help web applications. Let's dig into that.
  3. getUserMedia/applyConstraints have similar resource limitation concerns but we do not say anything there AFAIK. Maybe we should say something there as well, or maybe this is not in practice a big deal. Let's clarify this.
  4. It seems that this PR is at least partially trying to cover a limitation of Chrome's current implementation with regards to clone/cropTo. From what I understood, this limitation is more a bug than a desired behavior. I do not feel this alone justifies changing the spec, especially this early in the specification/implementation process where the trade-off end goal/reality is a bit more on the end goal side of things.

@jan-ivar
Copy link
Member

jan-ivar commented Jun 2, 2022

I believe @dontcallmedom and @aboba are working on clarifying rules of engagement in github issues for our WG. We have an interim meeting on Tuesday, so I'm going to focus on slides for #17. I encourage others to contribute slides on that issue as well.

To me, the agenda looks light, so maybe there would be time to present this issue as well if you wish to prepare slides?

@eladalon1983
Copy link
Member Author

  1. This PR needs more work to be merged. I made for instance the comment that the new check should be done asynchronously. @jan-ivar mentioned a different error type would be better.

Thanks. I've made that change.

  1. My position on this particular issue is not set yet

Then I have misunderstood you during the meeting. Apologies for misrepresenting your position here.

  1. ...

I don't think we rushed anything yet.
This PR is indeed only two days old, so go ahead and take your time.

  1. This PR does not seem like a blocker for shipping Chrome implementation (?)

Probably not.

  1. There are some arguments for/against this PR, it seems worth evaluating them. Let's take the time to discuss these arguments before merging the PR instead of after.

Let's.


  1. ... Maybe this is the first time we are trying to do so and we try to create a pattern. Or maybe this is an existing pattern that has been discussed and validated in other specs and we are just applying it here. ...

I am not sure, but hopefully people more experienced than me would know.

  1. Resource limitation is quite vague in general so it is unclear how this helps interoperability or how this is supposed to help web applications. Let's dig into that.

I see it as intentionally general, covering any implementation-specific limitation that prevents a browser from changing cropping at a given time. It gives developers predictability and interoperability in detecting otherwise-unexpected errors, without requiring us to discuss every newly-discovered implementation issue we encounter in the future. (If certain errors do become interesting enough to merit special treatment, we can specify later that these specific issues get handled differently.)

Maybe we should say something there as well,

Maybe. Wdyt? Should we?

or maybe this is not in practice a big deal.

Even if it's not a big deal, specifying it seems better than leaving it unspecified. See more below.

I do not feel this alone justifies changing the spec

If other implementations run into their own set of idiosyncratic limitations, they will be glad to discover that there is a predictable way of failing, and that developers are already aware of that type of failure. When the spec is aligned with implementations, everyone benefits.


I believe @dontcallmedom and @aboba are working on clarifying rules of engagement in github issues for our WG

That's great. Please let me know if there's anything in the engagement around this issue thus far, that you think could be improved.

We have an interim meeting on Tuesday, so I'm going to focus on slides for #17.

I am not sure I'll be able to attend. I regret that the intention to discuss #17 was announced only today, or I would have cleared my schedule ahead of time. (As of earlier today, no agenda items were announced.) I'll try to shuffle around my other commitments and make it. But if I cannot, I suggest that it might be more productive to discuss Region Capture during an interim meeting which I can attend. For your consideration.

<li>
If the user agent is unable to change the [=crop-state=] of the
{{MediaStreamTrack}}, for example due to resource limitation, the user agent MUST
return a new {{Promise}}, [=rejected=] with an {{OperationError}}.

Choose a reason for hiding this comment

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

(editorial) shouldn't we say "reject p and return it", given that it was created before the parallel steps?

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.

4 participants