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

Use the constrainable pattern instead of creating a subtly different function set #124

Closed
alvestrand opened this issue Dec 7, 2016 · 25 comments

Comments

@alvestrand
Copy link
Contributor

The combination of getPhotoCapabilities (with both ranges and current settings) and setOptions creates an almost equivalent set of functionalities to the constrainable pattern used on MediaStreamTrack.

Two nearly identical patterns is a bad thing for the platform. Please reconsider.

(This has been discussed before, and I know giri does not agree with me. I wish for the discussion to be recorded as a top-level issue in this tracker.)

@stefhak
Copy link

stefhak commented Dec 14, 2016

Asking in particular (I think) @gmandyam and @miguelao : what would the drawback(s) be of aligning with mediacapture-main and use the constrainable pattern?

@gmandyam
Copy link
Collaborator

@stefhak

I did provide my view on applicability of constraints to ImageCapture in the discussion on #73. In addition, I would note the following. Apologies for any clumsy wording.

a) The contraints mechanism is already leveraged in ImageCapture, as the constructor (https://w3c.github.io/mediacapture-image/#ImageCaptureAPI) uses a track to which constraints have been applied.

b) That being said, constraints (in my opinion) add value when there is some form of contention. By 'contention', I am describing a situation where for instance multiple tabs in the active browser context require media capture, each with their own individual requirements. However, for ImageCapture this kind of contention is not applicable as the ImageCapture object is constructed using a track that has already been created based on the browser constraint resolution.

c) It is still unclear (to me at least) whether developers have fully understood the constraints concept in the context of MediaStreams and actually found value in using them. In that respect, I am concerned about causing developer confusion by adding a constraints-on-top-of-constraints mechanism. For instance, developers may ask why they cannot apply image capture constraints directly to the media stream itself.

@stefhak
Copy link

stefhak commented Dec 15, 2016

@gmandyam I had not seen #73, thanks for the pointer.

A related question: has the core part of #73 (i.e. that some of the capabilities reported via getPhotoCapabilities() are actually settings) been addressed? Or is there no need (e.g. it is OK to report a setting because the capabilities are evident and there is never a need to probe for them)?

@jan-ivar
Copy link
Member

b) That being said, constraints (in my opinion) add value when there is some form of contention.

@gmandyam If two tabs are sharing the same camera and one calls:

new ImageCapture(stream.getVideoTracks()[0]).setOptions({ zoom: 4 });

and the other calls:

new ImageCapture(stream.getVideoTracks()[0]).setOptions({ zoom: 1 });

at the same time, what happens?

@gmandyam
Copy link
Collaborator

@jan-ivar

I can only answer from the driver perspective - in that case I believe it would be first come-first serve. But I will have to confirm internally.

@jan-ivar
Copy link
Member

@gmandyam Regardless of the answer, isn't this the same form of contention? How does zoom differ from, say resolution?

@gmandyam
Copy link
Collaborator

gmandyam commented Dec 16, 2016

@jan-ivar
No, I don't believe this is the same form of contention. If the underlying implementation (HW + platform + UA) cannot satisfy a setOptions() call at a given instant in time for one webpage because another webpage had invoked setOptions() on a mediaStream with the exact same setting (in this case zoom) at the same time, then that webpage can always implement its own logic to try to apply the setting after some delay.

What is the likelihood of two webpages in the active browser context trying to apply overlapping photo settings at the same time? If we think it is going to be a rare event, then constraints seems to be overkill to solve this problem.

@alvestrand
Copy link
Contributor Author

Giri,
your attempted resolution seems to indicate two things:

  • That ImageCapture options are used only for takePhoto()
  • That ImageCapture options are applied when takePhoto() is called, and are dropped when it's finished.

However, that's not what the spec says at all. Instead, it says of setOptions:

"it must queue a task, using the DOM manipulation task source, that runs the following steps:

  • Configure the underlying image capture device with the settings parameter."

I think you have indicated a deeper problem with the spec, but it's actually a problem unrelated to the issue of the shape of the API surface.

@gmandyam
Copy link
Collaborator

@alvestrand

The spec has a note in Sec. 2.3 that states the following:

"Devices may temporarily stop streaming data, reconfigure themselves with the appropriate photo settings, take the photo, and then resume streaming. In this case, the stopping and restarting of streaming should cause mute and unmute events to fire on the Track in question."

@jan-ivar
Copy link
Member

These statements are not mutually exclusive. It seems some settings get applied during takePhoto and others get applied immediately. zoom seems to be in the latter cagetory, and seems no different from a constraint in this case.

Btw, this isn't about inability to satisfy, but about describing needs in an uncontentious, way. e.g. the comparison should not be with getUserMedia but with applyConstraints.

See applyConstraints in action in Firefox (which doesn't do any rescaling and is limited to a single shared native camera output) in https://jsfiddle.net/ftdmdkjt/ - Try it in multiple tabs as well.

I don't think we can ignore adjacent tabs, as we're talking about a single shared hardware resource. We need to solve this because JS developers have no way to solve it.

@gmandyam
Copy link
Collaborator

@jan-ivar

I agree with your assessment, but my proposal was to have such settings that are prone to contention dealt with as constraints on the mediastream itself. In fact, I had suggested 'zoom' specifically as a constraint to be defined in https://www.w3.org/TR/mediacapture-streams/#media-track-supported-constraints, and was turned down by the group.

@jan-ivar
Copy link
Member

@gmandyam Are you suggesting we move all (how many are there?) immediately-applied settings to the track as constraints, and reserve setOptions for those that get set only during takePhoto()?

That could work. setOptions wouldn't need to return a promise anymore, or we could drop the method entirely in favor of an options argument to takePhoto.

@gmandyam
Copy link
Collaborator

@jan-ivar

Yes, that is what I am suggesting. In fact, it is not really a new suggestion on my part. When I suggested addressing 'zoom' as a constraint originally (see https://lists.w3.org/Archives/Public/public-media-capture/2013Sep/0136.html), I had this issue in mind. But this turned out to result in a contentious debate at the time.

My suggestion is to revisit constraints in the next version of both ImageCapture and Media Capture/Streams specs and address both specs with an eye as to what makes sense using the constraints mechanism and what makes sense using camera settings.

@jan-ivar
Copy link
Member

@gmandyam From your link, that debate seemed to be mostly about getting it into the Media Capture 1.0 spec. I see no reason the imageCapture spec, being an extension spec, can't define zoom as a track constraint.

The way zoom is defined right now seems broken and needs to be fixed, which is more urgent than a "revisit next version" thing IMHO.

@gmandyam
Copy link
Collaborator

@jan-ivar

I am fine with defining extensions to the MediaTrackConstraintSet dictionary in the context of ImageCapture, but will browser vendors support them?

@miguelao - what are your thoughts?

@jan-ivar
Copy link
Member

@gmandyam I prefer to view it as figuring out where zoom will live. People here are clamoring for constraints, so I don't see why not.

@jan-ivar
Copy link
Member

I agree with @alvestrand that two nearly identical patterns is a bad thing. But two constrainable objects isn't great either. Let's reuse the one we have. Move zoom.

I agree with @gmandyam there's no contention for settings effective only during takePhoto(where overlap causes UnknownError), making the constrainable pattern overkill for the remaining settings.

Solution: With zoom out of the picture (no pun intended), make takePhoto take an options object, and remove setOptions, then the patterns seem sufficiently different.

This streamlines the API by handling the two fundamentally different kinds of settings optimally.

@alvestrand
Copy link
Contributor Author

@gmandyam the issues with zoom as a constraint that were raised when you originally suggested it were the ones I tried to capture in #126 (which is closed, but where I haven't yet inspected the solution Miguel applied).

takePhoto with an options object would make the applicability of the configuration much clearer than the current pattern (where it seems to want to influence both takePhoto and grabFrame), so I'd support that.

I'm still unhappy with the two different syntaxes for specifying the parameters, and the two different APIs for getting possible settings and current settings.

@yell0wd0g
Copy link
Member

Just to note that there are two kinds of options, those which result is applicable immediately, and the user reasonably expects to see its effect in order to iterate towards a solution, and the others, whose effect is not visible until the exposure takes place.

This is a table with the settings and their immediacy, i.e. are they reflected immediately in the captured Stream or is the effect delayed to exposure time:

Setting
whiteBalanceMode immediate
colorTemperature immediate
exposureMode delayed
exposureCompensation immediate
iso delayed
redEyeReduction delayed
focusMode delayed
pointsOfInterest probably immediate
brightness immediate
contrast immediate
saturation immediate
sharpness immediate
imageHeight delayed
imageWidth delayed
zoom immediate
fillLightMode both

We could only pass the delayed ones to takePhoto(), all others need to be configurable separately: "live", if you want. IOW, that'd effectively create two ways of configuring, which seem undesirable. Moreover, zoom is not the only "live" setting.

Would it be unreasonable to follow the technique in CanvasCaptureMediaStreamTrack and define an ImageCaptureMediaStreamTrack where we could bundle all options, to be configured via a constraint pattern?

We'd still have the contention problem between the original MediaStreamTrack and the one controlled by ImageCapture, but that'd always be there because we are manipulating indirectly the hidden MediaStreamSource.

@jan-ivar
Copy link
Member

@miguelao s/zoom/all live settings/ in what I already said. That would be one way.
"Two ways of configuring" is unavoidable, unless you plan on re-adding width and height options.

I don't think we need to extend MediaStreamTrack to add constraints FWIW.

As to using constraints for delayed settings, what would you expect e.g. track.getSettings().exposureMode to return?

@yell0wd0g
Copy link
Member

@jan-ivar I'd like to see the concrete proposal of @alvestrand about how this all should look like if we were to
a) move the "live" settings to the track constraints (that track could only be of video type) and
b) pass all other constraints to takePhoto() (so grabFrame() would only reflect the "live" settings, which is the current behaviour of spec and implementations).

In particular, would a) look like an extension to MediaTrackSupportedConstraints? Would it be added to the Media Capture and Streams Spec (which is now CR)?

From the implementation POV, I'd love for this debate to be closed ASAP so I can adapt Chromium either way. @jan-ivar , what are the plans to implement takePhoto() and the settings in FF?

@jan-ivar
Copy link
Member

jan-ivar commented Dec 20, 2016

@alvestrand
Copy link
Contributor Author

@miguelao if the "user expects to see the effects in order to iterate towards a solution", that means he's modifying the mediastreamtrack that shows in his "viewfinder" video element, not the photos he's going to take.
In that case, you should be specifying additional constraints that can be applied to a mediastreamtrack. Adding constraints on a mediastreamtrack is an extension point in the mediastream-main spec.

@jan-ivar
Copy link
Member

Can this be closed now that #150 has merged?

@yell0wd0g
Copy link
Member

Yes! closing it.

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

No branches or pull requests

5 participants