Add support to get and set desired image format #185

alexshalamov · 2017-06-15T07:26:33Z

This PR adds possibility for the web developers to get formats supported by the UA and set desired format for the captured photo.

Fixes #162

Preview | Diff

alexshalamov · 2017-06-15T07:27:12Z

@YellowDoge please take a look.

yell0wd0g · 2017-06-15T22:38:45Z

index.bs

@@ -192,6 +192,7 @@ interface ImageCapture {
    readonly attribute MediaSettingsRange         imageHeight;
    readonly attribute MediaSettingsRange         imageWidth;
    readonly attribute FrozenArray&lt;FillLightMode> fillLightMode;
+    readonly attribute FrozenArray&lt;DOMString>     supportedImageFormats;


Could we add an entry to https://github.com/w3c/mediacapture-image/blob/master/implementation-status.md as well?

@YellowDoge I noticed that impl status.md has 'flattened' features for PhotoSettings and PhotoCapabilities, because attributes have same names. Should I rename 'image format' related attributes as well?

Hmm don't fully grasp the question, could you please rewrite it or provide an example...?

At the moment, PhotoCapabilities and PhotoSettings have same attribute names, even for arrays. For example PhotoCapabilities.fillLightMode represents supported fill light modes (supportedFillLightModes).

Question is that, what naming style should be used? If I follow current naming style, then PhotoCapabilities should have FrozenArray<DOMString> imageFormat attribute and PhotoSettings should have DOMString imageFormat attribute.

As the attribute names for PhotoCapabilities and PhotoSettings are the same (currently), implementation-status.md has just one column (Photo{Capabilities/Settings}).
If PhotoCapabilities and PhotoSettings would have different attribute names, it could be confusing and look like both have imageFormat and supportedImageFormats attributes.

Do you think implementation-status.md looks fine with my modifications?

I see what you mean, sorry I was so thick, thanks for the explanation. The other cases in the current Spec that follow this pattern of FrozenArray<DOMString> as capabilities and DOMString for setting/constraining have the same name in both cases, e.g. exposureMode and exposureMode, so I'd say that for the purpose of this PR is better to follow that idea, and just use the name imageFormats leaving to the dictionary to resolve the semantics.

If this still looks confusing we could consider changing all of them at the same time, but take into account that Cr already shipped those.

@YellowDoge thanks for comments, I renamed attribute to Photo{Capabilities/Settings}.imageFormats

yell0wd0g · 2017-06-15T22:50:32Z

index.bs

@@ -691,6 +699,9 @@ When the {{getSettings()}} method is invoked on a video stream track, the user a
    <li><dfn>Zoom</dfn> is a numeric camera setting that controls the focal length of the lens.  The setting usually represents a ratio, e.g. 4 is a zoom ratio of 4:1.  The minimum value is usually 1, to represent a 1:1 ratio (i.e. no zoom).</li>

    <li><dfn>Fill light mode</dfn> describes the flash setting of the capture device (e.g. |auto|, |off|, |on|). <dfn>Torch</dfn> describes the setting of the source's fill light as  continuously connected, staying on as long as {{track}} is active.</li>
+
+    <li><dfn>Image format</dfn> is the ASCII-encoded string in lower case describing the format of an image. The <a>image format</a> string MUST be a [=valid mime type=], whose [=type=] MUST be an [=image media type=] and [=subtype=] MAY be one of the [=IANA registered image subtypes=].


I think here we should specify that the UA should be able to display the format, similar to what we specify in e.g. MediaRecorder .

Added line similar to MediaRecorder

yell0wd0g · 2017-06-21T17:41:56Z

index.bs

@@ -240,6 +245,9 @@ interface ImageCapture {

  <dt><dfn dict-member for="PhotoSettings"><code>fillLightMode</code></dfn></dt>
  <dd>This reflects the desired <a>fill light mode</a> (flash) setting.</dd>
+
+  <dt><dfn dict-member for="PhotoSettings"><code>imageFormats</code></dfn></dt>
+  <dd>This reflects the desired <a>image format</a> for the captured photo. If the UA supports desired <a>image format</a>, the {{Blob/type!!attribute}} of the {{Blob}} produced by <a>takePhoto()</a> MUST be of the {{imageFormats!!dict-member}} [=image format|type=].</dd>


s/MUST be of/MUST be one of/

s/{{imageFormats!!dict-member}}/{{PhotoCapabilities/imageFormats}}/ ?

Also, despite [= =] syntax being valid, this document uses mostly the likes of <a>image format</a>.

s/MUST be of/MUST be one of/
s/{{imageFormats!!dict-member}}/{{PhotoCapabilities/imageFormats}}/ ?

Fixed, however, I would prefer original version. If web developer explicitly requests e.g., "image/tiff", proper type should be returned, or takePhoto() should fail if unsupported type was requested. Now, UA is allowed to return whatever it supports, regardless of provided PhotoSettings.

Also, despite [= =] syntax being valid, this document uses mostly the likes of image format.

Fixed. Is there a way to override display text for anchors? I use [==] in other specs that use bikeshed, as it is easier to make plurals or tailor text within different contexts.

I would prefer original version. If web developer explicitly requests e.g., "image/tiff", proper type should be returned, or takePhoto() should fail if unsupported type was requested.

PhotoSettings is used for both setting a configuration and readback of the current one, so the text has to be valid for both, leaving the actions to the methods using the dictionary, so that e.g. takePhoto({imageFormat: "image/invalid"}); should reject indeed, but when we read it back, then we should explicit that the image format should be one of the supported ones (and not, e.g. blank).

yell0wd0g · 2017-06-21T17:42:14Z

index.bs

@@ -209,6 +210,9 @@ interface ImageCapture {

  <dt><dfn attribute for="PhotoCapabilities"><code>fillLightMode</code></dfn></dt>
  <dd>This reflects the supported <a>fill light mode</a> (flash) settings, if any.</dd>
+
+  <dt><dfn attribute for="PhotoCapabilities"><code>imageFormats</code></dfn></dt>
+  <dd>This reflects [=image format|image formats=] supported by the UA.</dd>


s/reflects/reflects the/

yell0wd0g · 2017-06-21T19:56:35Z

index.bs

@@ -192,6 +192,7 @@ interface ImageCapture {
    readonly attribute MediaSettingsRange         imageHeight;
    readonly attribute MediaSettingsRange         imageWidth;
    readonly attribute FrozenArray&lt;FillLightMode> fillLightMode;
+    readonly attribute FrozenArray&lt;DOMString>     imageFormats;


s/imageFormats/imageFormat/ to follow the fillLightMode convention.

yell0wd0g · 2017-06-21T19:56:49Z

index.bs

@@ -223,6 +227,7 @@ interface ImageCapture {
    double          imageHeight;
    double          imageWidth;
    boolean         redEyeReduction;
+    DOMString       imageFormats;


Same here: s/imageFormats/imageFormat/ to follow the fillLightMode convention (and exposureMode etc).

yell0wd0g · 2017-06-21T20:04:05Z

index.bs

@@ -691,6 +699,9 @@ When the {{getSettings()}} method is invoked on a video stream track, the user a
    <li><dfn>Zoom</dfn> is a numeric camera setting that controls the focal length of the lens.  The setting usually represents a ratio, e.g. 4 is a zoom ratio of 4:1.  The minimum value is usually 1, to represent a 1:1 ratio (i.e. no zoom).</li>

    <li><dfn>Fill light mode</dfn> describes the flash setting of the capture device (e.g. |auto|, |off|, |on|). <dfn>Torch</dfn> describes the setting of the source's fill light as  continuously connected, staying on as long as {{track}} is active.</li>
+
+    <li><dfn>Image format</dfn> is the ASCII-encoded string in lower case describing the format of an image. The <a>image format</a> string MUST be a [=valid MIME type=], whose [=type=] MUST be an [=image media type=] and [=subtype=] MAY be one of the [=IANA registered image subtypes=]. The UA should be able to display any of the supported [=image format| image formats=].


s/[=valid MIME type=]/{{valid MIME type}}/ ? Throughout.

yell0wd0g

lgtm but let's see what @jan-ivar says.

yell0wd0g · 2017-06-22T15:49:31Z

index.bs

@@ -260,6 +265,9 @@ interface ImageCapture {

  <dt><dfn dict-member for="PhotoSettings"><code>fillLightMode</code></dfn></dt>
  <dd>This reflects the desired <a>fill light mode</a> (flash) setting.</dd>
+
+  <dt><dfn dict-member for="PhotoSettings"><code>imageFormat</code></dfn></dt>
+  <dd>This reflects the desired <a>image format</a> for the captured image. If the UA supports desired <a>image format</a>, the {{Blob/type!!attribute}} of the {{Blob}} produced by <a>takePhoto()</a> MUST be one of the {{PhotoCapabilities/imageFormat}} types.</dd>


I'd phrase the second sentence something like: When the UA returns a {{Blob}} from {{takePhoto()}}, the format and {{Blob/type}} MUST be one of the supported <a>image formats</a> enumerated in {{PhotoCapabilities/imageFormat}}, if any.

alexshalamov · 2017-07-25T07:38:32Z

@jan-ivar could you please check this PR when you have a time?

jan-ivar

Hi, back from a week off. Sorry for the delay!

jan-ivar · 2017-07-31T13:00:07Z

index.bs

@@ -229,6 +230,9 @@ interface ImageCapture {

  <dt><dfn attribute for="PhotoCapabilities"><code>fillLightMode</code></dfn></dt>
  <dd>This reflects the supported <a>fill light mode</a> (flash) settings, if any.</dd>
+
+  <dt><dfn attribute for="PhotoCapabilities"><code>imageFormat</code></dfn></dt>
+  <dd>This reflects the <a>image format</a> supported by the UA.</dd>


Maybe "This reflects the available <a>image format</a>s supported by the UA." - to capture that this is a list?

Do we want to add something like "The UA MUST support at least one image format"?

Thanks, will fix it as you proposed.

jan-ivar · 2017-07-31T14:55:13Z

index.bs

@@ -260,6 +265,9 @@ interface ImageCapture {

  <dt><dfn dict-member for="PhotoSettings"><code>fillLightMode</code></dfn></dt>
  <dd>This reflects the desired <a>fill light mode</a> (flash) setting.</dd>
+
+  <dt><dfn dict-member for="PhotoSettings"><code>imageFormat</code></dfn></dt>
+  <dd>When the UA returns a {{Blob}} from {{takePhoto()}}, the format and {{Blob/type}} MUST be one of the supported <a>image formats</a> enumerated in {{PhotoCapabilities/imageFormat}}, if any.</dd>


These look like normative steps for the takePhoto algorithm, and should probably move there.

As currently written it doesn't actually say the UA should look at the imageFormat setting at all. @YellowDoge said that "takePhoto() should fail if unsupported type was requested" - That would be one way to go. But which error? We'd need to specify that.

Sorry, was OOO.
@jan-ivar I think proper error handling is missing from takePhoto algorithm. IMO, takePhoto should reject promise with NotSupportedError if PhotoSettings with unsupported value is provided.

For this PR, I can align this text with other dictionary members like:
"This reflects the desired <a>image format</a> setting."

If you and @YellowDoge think that takePhoto should reject promise, maybe it would be better to do it in separate PR. What do you think?

Wfm. We should probably wait for #194 to merge before doing that anyway.

This PR adds possibility for the web developers to get formats supported by the UA and set desired format for the captured photo. Fixes w3c#162

alexshalamov · 2018-06-04T12:45:08Z

@jan-ivar #194 is merged, I've rebased this PR, could you please take a look?

jan-ivar

lgtm.

alexshalamov · 2018-06-06T06:37:28Z

@YellowDoge Could you please merge the PR if you are fine with the changes.

beaufortfrancois · 2020-07-01T11:33:59Z

@YellowDoge @riju What is the current status of this PR?

riju · 2020-08-20T12:56:14Z

I am missing some historical context and if there were specific customer asks @alexshalamov (no longer working at Intel) was privy to. IMO it looks like a good feature to have, but I think we should proceed with this only if there is proof of substantial customer interest, else we can keep this PR in this suspended state.

alvestrand · 2024-01-04T11:19:29Z

Closing due to lack of interest. We can always reopen if interest recovers.

yell0wd0g reviewed Jun 15, 2017

View reviewed changes

alexshalamov force-pushed the add_image_format branch from daa6ab6 to 92f5564 Compare June 21, 2017 11:21

yell0wd0g reviewed Jun 21, 2017

View reviewed changes

alexshalamov force-pushed the add_image_format branch 2 times, most recently from 594c54e to 563e6c7 Compare June 22, 2017 07:54

yell0wd0g reviewed Jun 22, 2017

View reviewed changes

yell0wd0g requested a review from jan-ivar June 22, 2017 15:49

alexshalamov force-pushed the add_image_format branch from 563e6c7 to 8d33d2e Compare June 26, 2017 14:07

jan-ivar requested changes Jul 31, 2017

View reviewed changes

alexshalamov force-pushed the add_image_format branch from 8d33d2e to 2d68434 Compare August 9, 2017 09:35

alexshalamov mentioned this pull request Nov 2, 2017

Fix in-parallel steps to make sense. #194

Merged

Add support to get and set desired image format

585f3e3

This PR adds possibility for the web developers to get formats supported by the UA and set desired format for the captured photo. Fixes w3c#162

alexshalamov force-pushed the add_image_format branch from 2d68434 to 585f3e3 Compare June 4, 2018 12:42

jan-ivar approved these changes Jun 5, 2018

View reviewed changes

riju mentioned this pull request Aug 19, 2020

Provide way to get / set captured image format #162

Closed

riju added the enhancement label Aug 19, 2020

Base automatically changed from master to main February 15, 2021 07:19

anonmos mentioned this pull request Apr 7, 2021

Optional MIME type as argument for takePhoto() #278

Open

alvestrand closed this Jan 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to get and set desired image format #185

Add support to get and set desired image format #185

alexshalamov commented Jun 15, 2017 •

edited by pr-preview bot

Loading

alexshalamov commented Jun 15, 2017

yell0wd0g Jun 15, 2017

alexshalamov Jun 19, 2017

yell0wd0g Jun 19, 2017

alexshalamov Jun 20, 2017

yell0wd0g Jun 20, 2017

alexshalamov Jun 21, 2017

yell0wd0g Jun 15, 2017

alexshalamov Jun 19, 2017

yell0wd0g Jun 21, 2017

alexshalamov Jun 22, 2017

yell0wd0g Jun 22, 2017 •

edited

Loading

yell0wd0g Jun 21, 2017

yell0wd0g Jun 21, 2017

yell0wd0g Jun 21, 2017

yell0wd0g Jun 21, 2017

yell0wd0g left a comment

yell0wd0g Jun 22, 2017

alexshalamov commented Jul 25, 2017

jan-ivar left a comment

jan-ivar Jul 31, 2017

alexshalamov Aug 7, 2017

jan-ivar Jul 31, 2017

alexshalamov Aug 7, 2017

jan-ivar Aug 7, 2017

alexshalamov commented Jun 4, 2018

jan-ivar left a comment

alexshalamov commented Jun 6, 2018

beaufortfrancois commented Jul 1, 2020

riju commented Aug 20, 2020

alvestrand commented Jan 4, 2024

Add support to get and set desired image format #185

Add support to get and set desired image format #185

Conversation

alexshalamov commented Jun 15, 2017 • edited by pr-preview bot Loading

alexshalamov commented Jun 15, 2017

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

yell0wd0g Jun 22, 2017 • edited Loading

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

yell0wd0g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexshalamov commented Jul 25, 2017

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

alexshalamov commented Jun 4, 2018

jan-ivar left a comment

Choose a reason for hiding this comment

alexshalamov commented Jun 6, 2018

beaufortfrancois commented Jul 1, 2020

riju commented Aug 20, 2020

alvestrand commented Jan 4, 2024

alexshalamov commented Jun 15, 2017 •

edited by pr-preview bot

Loading

yell0wd0g Jun 22, 2017 •

edited

Loading