split off from bug 691234 This is based on the proposal here: http://lists.w3.org/Archives/Public/public-media-capture/2012Mar/0008.html

Can we prefix the API with moz? The proposal still hasn't got consensus in the WG yet, and therefore is subject to change.

(In reply to Anant Narayanan [:anant] from comment #1) > Can we prefix the API with moz? The proposal still hasn't got consensus in > the WG yet, and therefore is subject to change. Adding Rags - Rags you suggested that we have to be able to tell app devs a standardized way. (The previous HTML5 input hack was standardized), is prefixing acceptable enough?

Comment on attachment 608566 [details] [diff] [review] patch Review of attachment 608566 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Navigator.cpp @@ +1002,5 @@ > +Navigator::GetUserMedia(IMediaStreamOptions *params, nsIDOMMediaCallback *callback) > +{ > + bool picture; > + if (!params || !callback || NS_FAILED(params->GetPicture(&picture)) || !picture) > + return NS_ERROR_NOT_IMPLEMENTED; returning error means throwing in javascript which kinda sucks. Lets not ever return from GetUserMedia. @@ +1011,5 @@ > + mozilla::AndroidBridge::Bridge()->ShowFilePickerForMimeType(filePath, NS_LITERAL_STRING("*/*")); > + nsILocalFile* file; > + NS_NewNativeLocalFile(NS_ConvertUTF16toUTF8(filePath), true, &file); > + callback->Callback(new nsDOMFileFile(file)); > + return NS_OK; this is all blocking. navigator.getUserMedia() must not block, right? ::: dom/interfaces/base/nsIDOMNavigator.idl @@ +158,5 @@ > * @param aTopic resource name > */ > nsIDOMMozWakeLock requestWakeLock(in DOMString aTopic); > + > + void getUserMedia(in IMediaStreamOptions params, in nsIDOMMediaCallback callback); If this method can not be implemented, should it throw or should the callback be called with a null blob? Or should there be an error callback like other dom APIs? Also, should this be prefixed?

(In reply to Doug Turner (:dougt) from comment #3) > Comment on attachment 608566 [details] [diff] [review] > patch > > Review of attachment 608566 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/base/Navigator.cpp > @@ +1002,5 @@ > > +Navigator::GetUserMedia(IMediaStreamOptions *params, nsIDOMMediaCallback *callback) > > +{ > > + bool picture; > > + if (!params || !callback || NS_FAILED(params->GetPicture(&picture)) || !picture) > > + return NS_ERROR_NOT_IMPLEMENTED; > > returning error means throwing in javascript which kinda sucks. Lets not > ever return from GetUserMedia. never return? huh? > > @@ +1011,5 @@ > > + mozilla::AndroidBridge::Bridge()->ShowFilePickerForMimeType(filePath, NS_LITERAL_STRING("*/*")); > > + nsILocalFile* file; > > + NS_NewNativeLocalFile(NS_ConvertUTF16toUTF8(filePath), true, &file); > > + callback->Callback(new nsDOMFileFile(file)); > > + return NS_OK; > > this is all blocking. navigator.getUserMedia() must not block, right? I haven't seen that specified > > ::: dom/interfaces/base/nsIDOMNavigator.idl > @@ +158,5 @@ > > * @param aTopic resource name > > */ > > nsIDOMMozWakeLock requestWakeLock(in DOMString aTopic); > > + > > + void getUserMedia(in IMediaStreamOptions params, in nsIDOMMediaCallback callback); > > If this method can not be implemented, should it throw or should the > callback be called with a null blob? Or should there be an error callback > like other dom APIs? doesn't seem to be specified. But since this is Anant's proposal, maybe he can clarify. > Also, should this be prefixed? see previous discussion

> never return? huh? No. You should return NS_OK unless you want to throw an exception to javascript. And you really shouldn't be throwing exceptions to javascript. > I haven't seen that specified Given that this method has a callback, I believe that the intent is that the call is asynchronous. > see previous discussion There was no conclusion.

(In reply to Brad Lassey [:blassey] from comment #4) > (In reply to Doug Turner (:dougt) from comment #3) > > > > this is all blocking. navigator.getUserMedia() must not block, right? > I haven't seen that specified As Doug mentions, the intent for the API is to be asynchronous. Callbacks usually imply this behaviour while functions that return values tend to be synchronous. > > If this method can not be implemented, should it throw or should the > > callback be called with a null blob? Or should there be an error callback > > like other dom APIs? > doesn't seem to be specified. But since this is Anant's proposal, maybe he > can clarify. I also prefer to have no exceptions in JavaScript, so in the case where no implementation exists, the error callback must be invoked. I omitted specifying the error callback because the message was sent as an addition to the larger getUserMedia spec which already had one. The full signature of the function is: navigator.getUserMedia({audio:false,video:false,picture:false}, successCB, errorCB); The success callback's only argument will be a Blob, while the error callback's only argument will be a string. "NOT_IMPLEMENTED" seems like a good place to start (the working group has rough consensus that we have error codes be strings and not enumerated constants). > > Also, should this be prefixed? > see previous discussion This API is *very* likely to change in the near future (there was a lot of follow up to the original proposal). The intent behind landing this code is: a) expose a simple camera API to developers without depending on the full-blown MediaStream based implementation so they can start building apps b) validate the API and general approach by getting feedback on the API from early developers Prefixing is one way of telling developers that an API is likely to change, but it's not the only way. I'm OK with either approach, for APIs that have not yet been standardised the usual path at Mozilla seems to be prefixing, but I don't see why we can't change the API later if we ship it without a prefix as long as we warn developers that it might.

Comment on attachment 608566 [details] [diff] [review] patch >+[scriptable, function, uuid(8e6cc130-d4dc-4751-a50c-ebdae4b17c02)] >+interface nsIDOMMediaCallback : nsISupports >+{ >+ void callback(in nsIDOMBlob blob); >+}; This part of the IDL assumes that the return argument type for the success callback will always be a Blob. However, we're working on the getUserMedia implementation for the desktop in bug 691234 where the type will a MediaStream if video or audio in MediaStreamOptions are set to true (and Blob if picture is set to true). It is an error condition [errorCB("INVALID_OPTIONS")] to have both picture and video set to true. Is it possible to describe this behaviour using our IDL system? If not, this is a consideration that will affect the design of the API, and we may have to rename this method to be something other than getUserMedia. The IDL over in bug 691234 (the desktop implementation) is essentially: +[scriptable, function, uuid(f2a144fc-3534-4761-8c5d-989ae720f89a)] +interface nsIDOMGetUserMediaSuccessCallback : nsISupports +{ + void onSuccess(in nsIDOMUserMediaStream stream); +}; + +[scriptable, function, uuid(2614bbcf-85cc-43e5-8740-964f52bdc7ca)] +interface nsIDOMGetUserMediaErrorCallback : nsISupports +{ + void onError(in string error); +}; + +[scriptable, uuid(05083c6a-a35c-4988-ba73-a226540ea5db)] +interface nsIDOMGetUserMedia : nsISupports +{ + void getUserMedia(in jsval options, + in nsIDOMGetUserMediaSuccessCallback success, + [optional] in nsIDOMGetUserMediaErrorCallback error); +}; We will want to be as close to this as possible.

Comment on attachment 611366 [details] [diff] [review] patch Review of attachment 611366 [details] [diff] [review]: ----------------------------------------------------------------- Do you have a spec/wiki on how users will use GetUserMedia()? f.e., can IMediaStreamOptions be optional? Also, what about other implementations? If this is just Android, you shouldn't expose mozGetUserMedia(). Also, we usually put platform logic outside of Navigator.cpp. If this get more complex (like supporting other platforms), you should move your code outside of this file. ::: dom/base/Navigator.cpp @@ +1014,5 @@ > +NS_IMETHODIMP > +Navigator::MozGetUserMedia(IMediaStreamOptions *params, nsIDOMMediaCallback *callback, nsIDOMErrorCallback *error) > +{ > + bool picture; > + if (!params || !callback || NS_FAILED(params->GetPicture(&picture)) || !picture) { what is the plan to support other media types? @@ +1016,5 @@ > +{ > + bool picture; > + if (!params || !callback || NS_FAILED(params->GetPicture(&picture)) || !picture) { > + if (error) > + error->Callback(NS_LITERAL_STRING("NOT_IMPLEMENTED")); You want to dispatch this instead of directly calling the Callback(). ::: dom/interfaces/base/nsIDOMNavigator.idl @@ +41,5 @@ > > +interface nsIDOMBlob; > + > + > +[scriptable, uuid(8a26205e-e8f7-4372-bd15-97ff982b4c1c)] one space between , and uuid @@ +42,5 @@ > +interface nsIDOMBlob; > + > + > +[scriptable, uuid(8a26205e-e8f7-4372-bd15-97ff982b4c1c)] > +interface IMediaStreamOptions : nsISupports IMediaStreamOptions probably should be nsIMediaStreamOptions, right? @@ +61,5 @@ > +{ > + void callback(in AString errorStr); > +}; > + > +[scriptable, uuid(3a4e42fb-fbf2-45f6-8841-a0c8d83bd558)] These headers should be in their own file(s) and not directly in nsIDOMNavigator.idl. @@ +164,5 @@ > * @param aTopic resource name > */ > nsIDOMMozWakeLock requestWakeLock(in DOMString aTopic); > + > + void mozGetUserMedia(in IMediaStreamOptions params, in nsIDOMMediaCallback callback, [optional] in nsIDOMErrorCallback error); You shouldn't directly add your method here. Instead do what bluetooth, sms, geolocation and telephony did. Define a new interface and have that extend the navigator implementation.

(In reply to Doug Turner (:dougt) from comment #10) > Comment on attachment 611366 [details] [diff] [review] > patch > > Review of attachment 611366 [details] [diff] [review]: > ----------------------------------------------------------------- > > Do you have a spec/wiki on how users will use GetUserMedia()? f.e., can > IMediaStreamOptions be optional? All I have is this http://lists.w3.org/Archives/Public/public-media-capture/2012Mar/0008.html and from that the answer would be no. > ::: dom/base/Navigator.cpp > @@ +1014,5 @@ > > +NS_IMETHODIMP > > +Navigator::MozGetUserMedia(IMediaStreamOptions *params, nsIDOMMediaCallback *callback, nsIDOMErrorCallback *error) > > +{ > > + bool picture; > > + if (!params || !callback || NS_FAILED(params->GetPicture(&picture)) || !picture) { > > what is the plan to support other media types? that's bug 691234 > These headers should be in their own file(s) and not directly in > nsIDOMNavigator.idl. They are now, but note that I'm still rev'ing nsIDOMNavigator's uuid for binary compat on Android

Comment on attachment 612110 [details] [diff] [review] patch Review of attachment 612110 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Navigator.cpp @@ +1002,5 @@ > return pmService->NewWakeLock(aTopic, win, aWakeLock); > } > > +#ifdef ANDROID > +class GetUserMediaCallback: public nsFilePickerCallback { This should be move out of this file. @@ +1008,5 @@ > + GetUserMediaCallback(nsIDOMMediaCallback* aCallback, nsIDOMErrorCallback *aError) : > + mCallback(aCallback) , mError(aError){} > + > + virtual void handleResult(nsAString& filePath) { > + nsILocalFile* file; this leaks. Use nsCOMPtr<nsILocalFile> @@ +1009,5 @@ > + mCallback(aCallback) , mError(aError){} > + > + virtual void handleResult(nsAString& filePath) { > + nsILocalFile* file; > + NS_NewNativeLocalFile(NS_ConvertUTF16toUTF8(filePath), true, &file); Don't convert. Use NS_NewLocalFile. @@ +1012,5 @@ > + nsILocalFile* file; > + NS_NewNativeLocalFile(NS_ConvertUTF16toUTF8(filePath), true, &file); > + bool exists; > + if (!file || NS_FAILED(file->Exists(&exists)) || !exists) { > + if (mError) style here is: if (test) { stmt(); } @@ +1042,5 @@ > +Navigator::MozGetUserMedia(nsIMediaStreamOptions *params, nsIDOMMediaCallback *callback, nsIDOMErrorCallback *error) > +{ > + bool picture; > + if (!params || !callback || NS_FAILED(params->GetPicture(&picture)) || !picture) { > + if (error) Style here too @@ +1043,5 @@ > +{ > + bool picture; > + if (!params || !callback || NS_FAILED(params->GetPicture(&picture)) || !picture) { > + if (error) > + NS_DispatchToMainThread(new ErrorCallbackRunnable(error, NS_LITERAL_STRING("NOT_IMPLEMENTED"))); Not the right error string in all cases. Make this more correct or more generic. ::: dom/base/Navigator.h @@ +134,4 @@ > NS_DECL_NSIDOMNAVIGATORBLUETOOTH > #endif > > +#ifdef ANDROID if we are going to implement this, I'd really like to see this implemented on more than just android. ::: dom/interfaces/base/nsIDOMNavigator.idl @@ +39,4 @@ > > #include "domstubs.idl" > > +[scriptable, uuid(3a4e42fb-fbf2-45f6-8841-a0c8d83bd558)] you do not need this change. there is no binary compat on android for this file. ::: dom/interfaces/base/nsIDOMNavigatorUserMedia.idl @@ +12,5 @@ > +interface nsIMediaStreamOptions : nsISupports > +{ > + attribute boolean audio; > + attribute boolean video; > + attribute boolean picture; these should be readonly, right?

(In reply to Doug Turner (:dougt) from comment #12) > ::: dom/base/Navigator.h > @@ +134,4 @@ > > NS_DECL_NSIDOMNAVIGATORBLUETOOTH > > #endif > > > > +#ifdef ANDROID > > if we are going to implement this, I'd really like to see this implemented > on more than just android. Implementation for all desktop platforms (Windows, Mac, Linux) is being tracked in 691234. That bug is blocking on landing the WebRTC libraries into mozilla-central which is a bit more complicated - on Android we have the backend ready to go, and hence we'd like to land this feature on mobile first. Additionally, since this is not a standard API (yet), the earlier we ship it (even if just on mobile), the earlier we can get developer feedback. However, as I noted in comment #7, it would be nice to design the IDL that allows for a smooth transition to the desktop implementation that we are working on in bug 691234.

anant, sounds good.

Try run for 075bf5cc7f8f is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=075bf5cc7f8f Results (out of 223 total builds): success: 165 warnings: 54 failure: 4 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-075bf5cc7f8f

Try run for 740254267989 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=740254267989 Results (out of 133 total builds): success: 105 warnings: 21 failure: 7 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-740254267989

Try run for 7024e25af2be is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=7024e25af2be Results (out of 217 total builds): exception: 1 success: 180 warnings: 35 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-7024e25af2be

Try run for 84280d60e605 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=84280d60e605 Results (out of 222 total builds): success: 187 warnings: 35 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/blassey@mozilla.com-84280d60e605 Timed out after 12 hours without completing.

just to note, the ifdef's in Navigator.h and Navigator.cpp needed to be changed from ANDROID to MOZ_WIDGET_ANDROID in order to not break b2g

Why are we in a hurry to implement this API? It appears to me that this does less than <input type=file> does. Have you guys tested <input type=file> in new Fennec builds? Nowadays <input type=file> uses intents to display a nice picker which lets you choose which application you want to use to select a file. You can either use it to select any type of files (including video and audio) by just providing the above markup. If you want to only show intents which provides pictures then the following markup works: <input type=file accept="image/*"> Likewise you can request only videos <input type=file accept="video/*"> or only audio using <input type=file accept="audio/*">. If the page doesn't like the default UI (which is definitely terrible) they can always do their own UI and just call myInputElement.click(). The advantage this "API" has over getUserMedia is that it works fairly well across platforms. <input type=file> works both in Chrome with basically the exact same UI as we have in Fennec, as well as on all desktop browsers (by bringing up a file picker). It seems like we will have much better adoption by using this existing API rather than trying to push a new one. At least until the new one provides functionality which the old one doesn't. Am I missing something?

This API is a programmatic way of doing the same thing <input type=file> does, while serving as the tier-1 implementation of the full getUserMedia specification.

Comment on attachment 612441 [details] [diff] [review] patch Review of attachment 612441 [details] [diff] [review]: ----------------------------------------------------------------- I'm not really a fan of taking separate success and error callbacks. A much nicer syntax would be something like: getUserMedia({ picture: true }, { onsuccess: function(blob) {...}, onerror: function(errorstring)}); Alternatively you could return a DOMRequest. Since I'm heading on vacation, feel free to find another person to sr once you've made either of these changes. ::: dom/base/NavigatorUserMediaAndroid.cpp @@ +47,5 @@ > + const nsString mErrorMsg; > +}; > + > +NS_IMETHODIMP > +Navigator::MozGetUserMedia(nsIMediaStreamOptions *params, nsIDOMMediaCallback *callback, nsIDOMErrorCallback *error) This will be a lot simpler if you use a dictionary and our automatic dictionary parsing (this is more future proof as we move to new DOM bindings as well). Here's an example of how to do it: http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIDOMFile.idl#110 http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMBlobBuilder.cpp#207 ::: dom/interfaces/base/nsIDOMNavigatorUserMedia.idl @@ +8,5 @@ > + > +interface nsIDOMBlob; > + > +[scriptable, uuid(8a26205e-e8f7-4372-bd15-97ff982b4c1c)] > +interface nsIMediaStreamOptions : nsISupports Use a dictionary instead.

(In reply to Jonas Sicking (:sicking) from comment #23) > I'm not really a fan of taking separate success and error callbacks. A much > nicer syntax would be something like: > > getUserMedia({ picture: true }, { onsuccess: function(blob) {...}, onerror: > function(errorstring)}); > > Alternatively you could return a DOMRequest. The option that is currently being discussed in the working group is a single node.js style callback: getUserMedia({picture: true}, function(err, blob) {...}); If we are going to deviate from the spec, I suggest we implement the single callback. The advantage of this approach is that it forces the developer to check for error conditions (which is never almost done with two callbacks or even onsuccess/onerror). And if the developer doesn't check for errors, use of blob will atleast result in an exception (since it will be null) with a reasonable stack trace, whereas in the other two cases failure is silent (if the developer didn't provide an error callback).

Please let's not use the node.js style, I've heard nothing but complaints about that. the { onsuccess: ..., onerror: ... } style matches what JS libraries does I believe. That's a much better precedence for the web. DOMRequests is what we use for a lot of APIs in WebAPI, it also matches IndexedDB and is similar to XMLHttpRequest/FileReader.

(In reply to Jonas Sicking (:sicking) from comment #21) > Why are we in a hurry to implement this API? > > It appears to me that this does less than <input type=file> does. Have you > guys tested <input type=file> in new Fennec builds? personally, I think <input type=file> is sufficient. Unassigned myself

The original reason for wrapping input type=file with this is because in future the thought was we need more than simple still picture capture (ie live preview) and we wanted to provide a forward compatible path for app developers. From the mobile implementer side it would be good to know Jonas if you have fundamental objections to getUserMedia or if we just need to refine it a bit.

Sorry I didn't respond here earlier. Replying to JP's comment from weeks ago: >Adding Rags - Rags you suggested that we have to be able to tell app devs a >standardized way. (The previous HTML5 input hack was standardized), is >prefixing acceptable enough? Please realize that the previous Media Capture API that <input type=file> comes from has been deprecated in favor of getUserMedia: http://www.w3.org/2009/dap/wiki/ImplementationStatus. (Although the directory name in the link says "2009", this page is kept pretty up-to-date.) Even Microsoft who had been pushing the previous Media Capture API based on <input type=file>, has transitioned to focusing on getUserMedia: http://msoftnews.com/uncategorized/media-capture-api-helping-web-developers-directly-import-image-video-and-sound-data-into-web-apps/ To quote Anant from 2 weeks ago: The reason to get the getUserMedia image capture patch in right now is to get our proverbial foot in the door. The full scale getUserMedia implementation is likely to take a bit longer so implementing this in the short term gives us two things: - Tells the world that we are serious about getUserMedia and that we think it is the API to use for all image/video capture needs going forward. - Provide a programmatic *complement* to the declarative <input> API and gives us early developer feedback on the API." No one is suggesting that we remove the <input> implementation from our code. Jonas and I talked last month on irc about adding it to desktop shortly after we add desktop support for getUserMedia (which is currently part of the WebRTC project) which I hope will be in the code in the FF16 time frame. This discussion is very similar to the discussion we had last month, where we decided, "yes, let's add getUserMedia to our arsenal and help drive the getUserMedia standard by putting the first piece, still image capture, as a prefixed API into our code." Standards groups are notorious for taking a long time to make decisions. Having working code in our browser is a very effective way to push a standard forward. One can say, "look -- it's working already in this prefixed API." From a practical perspective: several of us have already taken the decision from last month and run with it. Having Camera API using getUserMedia in Android and B2G is a Q2 goal for Media team. (There were multiple posts about this on dev.media.) In fact for the past two weeks, Lucas, Ragavan, Jen Arguello, and I have been driving the Camera API - getUserMedia UX discussions forward in meetings and on the public lists. What's more, our Marketplace folks are telling our Apps developers that getUserMedia is our API for camera apps. It will be used not only for still image phones, but (more interestingly) for live preview once we have other things landed like Roc's MediaStreams code. Being able to tell apps developers that there is one API (even if programmatically they are different calls/interfaces) is a marketing advantage. So changing directions and effectively abandoning this API for still image capture have huge ramifications and implications which really scare me. Regarding technical objections that I see here: 1) I believe anything that we are doing in <input type=file>, we can do in getUserMedia. Is that correct, Anant and Brad? So we can add the ability to select audio and video files (in addition to images), which I would love to have, but it is not a requirement for Q2 or for K9o. 2) The Media Capture Task Force is serious about adding still image capture. They just haven't agreed on the API. Anant's proposal is the working one at this point. Implementing it as a prefixed API (which by it's nature is subject to change) will move those discussions and the spec work forward. If there's a point or objection I didn't discuss, please let me know. Or feel free to come find me on IRC (in #media, nick:mreavy). But we really need this to get landed in FF15 for all the reasons I cited above.

Ugh, I hate that we keep talking past each other. I feel like the we're having the same conversations over and over. To be clear. I'm ok with adding getUserMedia for still images. I even think that long term such an API is inevitable simply because some people prefer "pure JS" APIs, and also because we can't expose the <input>-based API to WebWorkers. That said, I that when we do getUserMedia for still images, we should do it right. It feels to me that we're currently rushing in an API for what I can tell is no good reason. Let me comment on a few of the statements below. > Please realize that the previous Media Capture API that <input type=file> > comes from has been deprecated in favor of getUserMedia: > http://www.w3.org/2009/dap/wiki/ImplementationStatus. (Although the > directory name in the link says "2009", this page is kept pretty > up-to-date.) I'm not quite following what you are referring to when you say "previous Media Capture API that <input type=file> comes from". There has been a lot of different proposals for how to do media capturing. I couldn't find any links to any specifications based on <input type=file> on that page. However it is simply *not* the case that media capture using <input type=file> has been deprecated. The functionality is still alive and well in the HTML5 specification. We would do both the web and ourselves a disservice by claiming that getUserMedia should supersede <input type=file>. I know there was one <input type=file>-based spec-draft from W3C that got discontinued. The main problem with that specification was that it solved non-problems. I.e. it tried to add features to <input type=file> which it simply already had or didn't need. The deprecation of that specification was in no way a deprecation of <input type=file>. There are literally millions of pages out there which would immediately get camera support if we spent the time investing in <input type=file>. We simply can't get the same adoption for getUserMedia within a reasonable time frame. > Even Microsoft who had been pushing the previous Media Capture API based on > <input type=file>, has transitioned to focusing on getUserMedia: > http://msoftnews.com/uncategorized/media-capture-api-helping-web-developers- > directly-import-image-video-and-sound-data-into-web-apps/ I quite strongly suspect this depends on who you ask at microsoft. I'll also note that Google has deployed <input type=file> media capture support in chrome for android, whereas they have not done so for getUserMedia. > To quote Anant from 2 weeks ago: > > The reason to get the getUserMedia image capture patch in right now is > to get our proverbial foot in the door. The full scale getUserMedia > implementation is likely to take a bit longer so implementing this in the > short term gives us two things: > > - Tells the world that we are serious about getUserMedia and that we > think it is the API to use for all image/video capture needs going forward. I disagree with this. getUserMedia and <input type=file> are both good tools for media capture. We should invest in and support both of them. If we have concerns about spreading ourselves too thin, then please see above about current deployment of <input type=file> vs. current deployment of getUserMedia. Also, is anyone really thinking that we're not serious about getUserMedia? Why are we worried that people don't take our commitment to this new standard seriously. This isn't something we've worried about for various APIs in the past. > - Provide a programmatic *complement* to the declarative <input> API and > gives us early developer feedback on the API." This I agree with. Though please note that it's a *very* small part of the streaming support that we'll get feedback on. The proposed getUserMedia API for still images is dramatically different from the streaming getUserMedia API. Developers that use the still-image getUserMedia will have to completely rewrite their code in order to use the streaming getUserMedia. > This discussion is very similar to the discussion we had last month, where > we decided, "yes, let's add getUserMedia to our arsenal and help drive the > getUserMedia standard by putting the first piece, still image capture, as a > prefixed API into our code." > > Standards groups are notorious for taking a long time to make decisions. > Having working code in our browser is a very effective way to push a > standard forward. One can say, "look -- it's working already in this > prefixed API." Agreed. So let's do it right rather than rushing something in, otherwise we won't get actual useful feedback. > What's more, our Marketplace folks are telling our Apps developers that > getUserMedia is our API for camera apps. I think we should stop doing this. We should be telling people to use the right tools for the right job. I.e. the right API for the right use cases. > It will be used not only for still > image phones, but (more interestingly) for live preview once we have other > things landed like Roc's MediaStreams code. Indeed. But see above that the live-preview API is a *very* different API, even though both use getUserMedia. It will require complete rewrite of the code that gets the media. I don't think we can claim that still-image-getUserMedia is any sort of migration path. > 1) I believe anything that we are doing in <input type=file>, we can do in > getUserMedia. Is that correct, Anant and Brad? This is incorrect. <input type=file> is better for accessibility since the it allows screen readers to tell the user that input for a file and/or image is requested. > So we can add the ability to select audio and video files (in addition to > images), which I would love to have, but it is not a requirement for Q2 or > for K9o. This is already working with <input type=file> for what it's worth. > 2) The Media Capture Task Force is serious about adding still image capture. > They just haven't agreed on the API. Anant's proposal is the working one at > this point. Implementing it as a prefixed API (which by it's nature is > subject to change) will move those discussions and the spec work forward. Agreed.

(In reply to JP Rosevear [:jpr] from comment #27) > The original reason for wrapping input type=file with this is because in > future the thought was we need more than simple still picture capture (ie > live preview) and we wanted to provide a forward compatible path for app > developers. From the mobile implementer side it would be good to know Jonas > if you have fundamental objections to getUserMedia or if we just need to > refine it a bit. I don't have fundamental objections (sorry, I wasn't clear enough on that in my super review). However I wouldn't call this a "forward compatible path" since the way you'd use the getUserMedia API proposed here is dramatically different from the streaming getUserMedia API.

So I talked to dherman about callback styles and he talked me out of the callback-style that I was advocating in comment 23. Basically the style we would ideally like is to return a promise. However promises are far from being standardized anywhere and so we can't go that route. The best to options that we agreed on are: getUserMedia({ picture: true }, successHandler, errorHandler); // this is what the patch already does or getUserMedia({ picture: true, onsuccess: successHandler, onerror: errorHandler}); The latter has the advantage that you can abstract out all your picture handling in one class and do: getUserMedia(new PictureHandler()); I'll write up a patch for the latter.

(In reply to Jonas Sicking (:sicking) from comment #29) > To be clear. I'm ok with adding getUserMedia for still images. I even think > that long term such an API is inevitable simply because some people prefer > "pure JS" APIs, and also because we can't expose the <input>-based API to > WebWorkers. Good, we're on the same page here! > That said, I that when we do getUserMedia for still images, we should do it > right. It feels to me that we're currently rushing in an API for what I can > tell is no good reason. The primary driver for rushing this API is the launch of our apps offering which is coming at the end of Q2. Consequently, we'd like to be able to point at a JS API to developers to say "here's how you access the camera, more coming soon", ideally in Gecko 15. This is really important for our "web apps can do everything native apps can" message. > Agreed. So let's do it right rather than rushing something in, otherwise we > won't get actual useful feedback. I'd love your inputs on how we can improve the API. 100% agreed on "let's do this right". But I do want to keep the FF15 tree closure front and center as we continue to work on this. June 5th. Let's do the best we can!

(In reply to Jonas Sicking (:sicking) from comment #31) > getUserMedia({ picture: true, onsuccess: successHandler, onerror: > errorHandler}); This would be a neat style, however, we should also keep in mind that we're going to be adding constraints and capabilities to the first argument soon. I feel like passing a single argument with several properties (some of them optional), may not be the best idea. The constraints proposal is here - http://lists.w3.org/Archives/Public/public-media-capture/2012Mar/0033.html - needs some cleanup, but there is general consensus on the idea in the WG. I'd prefer we implement the (args, onSuccess, onError) style, for this first round. The nice thing about that is that we'd still be compatible with the spec function signature wise, and the non-standard {picture:true} is sneaking behind the args. > The latter has the advantage that you can abstract out all your picture > handling in one class and do: > > getUserMedia(new PictureHandler()); Hmm, this does seem nice! I'm still a little skeptical of having a single object argument with more than 4-5 properties though...

It seems to me like we must strike a balance between too many properties in an object and too many function arguments. With BrowserID we do (opts, success, error) too, as the options argument all contain non-function values. It feels like a clean enough separation (functions stay as arguments, everything else goes in a property?)

> The primary driver for rushing this API is the launch of our apps offering > which is coming at the end of Q2. Consequently, we'd like to be able to > point at a JS API to developers to say "here's how you access the camera, > more coming soon", ideally in Gecko 15. This is really important for our > "web apps can do everything native apps can" message. Again, this argument doesn't make any sense to me. We aren't adding *any* new capabilities with this patch. So the "web apps can do everything native apps can" message isn't getting any stronger. Nor are we adding any migration paths given that any still picture API is going to be dramatically different from any streaming API. > > The latter has the advantage that you can abstract out all your picture > > handling in one class and do: > > > > getUserMedia(new PictureHandler()); > > Hmm, this does seem nice! I'm still a little skeptical of having a single > object argument with more than 4-5 properties though... Why is having many properties on a single object a problem? It seems to me that as you add even more options to the function call it gets harder to keep track of what is what and so labeling them becomes even more important. Additionally, as we add more capabilities to the API, it seems to me that the options passed in the first argument will be more tightly tied to the success and error callbacks, and so keeping them in the same object makes even more sense. Compare to the jQuery AJAX API which takes a settings argument with *a lot* of options: http://api.jquery.com/jQuery.ajax/

For what it's worth, I'm building a patch with the newly suggested API now.

(In reply to Jonas Sicking (:sicking) from comment #35) > > The primary driver for rushing this API is the launch of our apps offering > > which is coming at the end of Q2. Consequently, we'd like to be able to > > point at a JS API to developers to say "here's how you access the camera, > > more coming soon", ideally in Gecko 15. This is really important for our > > "web apps can do everything native apps can" message. > > Again, this argument doesn't make any sense to me. We aren't adding *any* > new capabilities with this patch. So the "web apps can do everything native > apps can" message isn't getting any stronger. Nor are we adding any > migration paths given that any still picture API is going to be dramatically > different from any streaming API. The <input> API isn't available on Desktop, while we are working on a getUserMedia implementation. I'm not aware of any plans for implementing <input> on Desktop. It would be nice to be able to show the same API for both Android & Desktop on Apps launch day! > Why is having many properties on a single object a problem? It seems to me > that as you add even more options to the function call it gets harder to > keep track of what is what and so labeling them becomes even more important. I agree about labels, my only nit with including functions in the object is that it is a *minor* stylistic win when you declare the callbacks inline when they are separate arguments as opposed to an object property. But, this is mostly a matter of personal preference (as most style-related questions are!) so I'm OK with implementing any style. I'll just express my preference for the functions-as-arguments approach.

(In reply to Anant Narayanan [:anant] from comment #37) > The <input> API isn't available on Desktop, while we are working on a > getUserMedia implementation. I'm not aware of any plans for implementing > <input> on Desktop. It would be nice to be able to show the same API for > both Android & Desktop on Apps launch day! This is bug 692955, that has a wip patch that worked until very recently... > I agree about labels, my only nit with including functions in the object is > that it is a *minor* stylistic win when you declare the callbacks inline > when they are separate arguments as opposed to an object property. > > But, this is mostly a matter of personal preference (as most style-related > questions are!) so I'm OK with implementing any style. I'll just express my > preference for the functions-as-arguments approach. Since we are in the style department, I'd like that: DOMRequest getUserMedia({ video: true; ...})

(In reply to Fabrice Desré [:fabrice] from comment #38) > (In reply to Anant Narayanan [:anant] from comment #37) > > > The <input> API isn't available on Desktop, while we are working on a > > getUserMedia implementation. I'm not aware of any plans for implementing > > <input> on Desktop. It would be nice to be able to show the same API for > > both Android & Desktop on Apps launch day! > > This is bug 692955, that has a wip patch that worked until very recently... Nice! We should definitely try and get that landed before FF15, if not both <input> and gUM. We should be able to reuse the same UI for both as well...

Also, it is no easier to add getUserMedia support to Desktop than it is to add basic <input type=file> support on desktop. I agree that returning a DOMRequest would be a nice solution too. That would be consistent with a pile of APIs that we're doing in WebAPI, as well as with IndexedDB and FileHandle. The main reason I'm not pushing for DOMRequest here is actually to get feedback on different styles of doing callbacks.

This implements the getUserMedia({ picture: true, onsuccess: ..., onerror: ...}) syntax. It also fixes some error handling in the original patch (we should throw synchronously if the first argument is missing or if any of its getters throw, any of those situations is just page bugs). This is untested as I don't have a android test environment here.

Use smaug's dictionary code, please.

Jonas, are there any other aspects of the API you'd like to see improve in the first iteration, or are you OK with checking-in the API in attachment 619839 [details] [diff] [review] (possibly on multiple platforms, but Android first)?

Ms2ger: Since it has callback functions it isn't a dictionary (and can't be since we need to keep a reference to the object as to get the 'this' parameter correct). Anant: Yes, I'm ok with checking in the API in attachment 619839 [details] [diff] [review]. I'd be ok with the earlier patch too (though we should fix it to throw exceptions for a couple of things, as well as change it to use the dictionary code) or returning a DOMRequest. I'm still partial to using attachment 619839 [details] [diff] [review] or returning a DOMRequest though, but that's a personal preference.

Ugh, crap, I realized that we should also integrate this with the popup blocker so that the page can't hi-jack the user's browser. We already do this for basically all popups like file pickers, print dialogs, alert()s etc. Check here for how to do that: http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp?force=1#249 http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp?force=1#272

Will refactor this patch to use the cross-platform DOM bindings we're working on in bug 752353, and add the popup blocking code as suggested by Jonas.

This patch makes use of the new Media Engine backend infrastructure (bug 750943 and bug 752351), as well as the DOM bindings in bug 752353. It enables navigator.mozGetUserMedia by default on Android only. A try run with all the pre-requisite patches for this one is at: https://tbpl.mozilla.org/?tree=Try&rev=7a0d29d516c1

Comment on attachment 629047 [details] [diff] [review] Android support for still picture capture Review of attachment 629047 [details] [diff] [review]: ----------------------------------------------------------------- Jonas would be a better reviewer

Over in bug 750943, we changed the Media Engine interface to take an nsIDOMFile instead of a nsILocalFile.

Comment on attachment 629106 [details] [diff] [review] Android support for still picture capture v2 Review of attachment 629106 [details] [diff] [review]: ----------------------------------------------------------------- content/media/MediaEngineDefault.cpp doesn't seem to be in the current version of m-c. In which patch is it added? ::: content/media/MediaEngineDefault.cpp @@ +157,5 @@ > { > +#ifdef ANDROID > + nsAutoString filePath; > + nsCOMPtr<nsILocalFile> localFile; > + if (mozilla::AndroidBridge::Bridge()) { It is usually preferred to do if (!foo) { return NS_OK; } instead of: if (foo) { // stuff } return NS_OK; Also, you do not seem to need |mozilla::|, as far as I can see, you are already in the mozilla namespace. @@ +161,5 @@ > + if (mozilla::AndroidBridge::Bridge()) { > + mozilla::AndroidBridge::Bridge()->ShowFilePickerForMimeType(filePath, NS_LITERAL_STRING("image/*")); > + nsresult rv = NS_NewLocalFile(filePath, false, getter_AddRefs(localFile)); > + NS_ENSURE_SUCCESS(rv,rv); > + *aFile = new nsDOMFileFile(localFile); That needs to be addref'd. @@ +162,5 @@ > + mozilla::AndroidBridge::Bridge()->ShowFilePickerForMimeType(filePath, NS_LITERAL_STRING("image/*")); > + nsresult rv = NS_NewLocalFile(filePath, false, getter_AddRefs(localFile)); > + NS_ENSURE_SUCCESS(rv,rv); > + *aFile = new nsDOMFileFile(localFile); > + return rv; |return NS_OK;| rv can't be something else at that point. @@ +169,3 @@ > *aFile = nsnull; > return NS_ERROR_NOT_IMPLEMENTED; > } A few general comment about this method: You should do: method() { #ifndef ANDROID *aFile = nsnull; return NS_ERROR_NOT_IMPLEMNTED; #else // Show a file picker with filter being "image/*". #endif } To show the filepicker, see: AsyncClickHandler::Run() in nsHTMLInputElement.cpp. Showing a generic filepicker instead or directly calling the Android backend method will have the same behavior on Android but will prevent you from including Android specific stuff and will make possible for other platforms to implement the same feature easily. BTW, you are aware that this will show an intent-like dialog with the list of all apps able to pick images (like the Gallery app) *and* all apps able to take a picture. ::: dom/base/Navigator.cpp @@ +915,1 @@ > if (!Preferences::GetBool("media.enabled", false)) { Unless there is a strong reason to do that, you should instead set "media.enabled" to true by default for Native Fennec in mobile/android/app/mobile.js

Per Fabrice, all these should probably be MOZ_WIDGET_ANDROID not ANDROID

Comment on attachment 629356 [details] [diff] [review] Android support for still picture capture v3 Review of attachment 629356 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaEngineDefault.cpp @@ +166,5 @@ > + nsresult rv = NS_NewLocalFile(filePath, false, getter_AddRefs(localFile)); > + NS_ENSURE_SUCCESS(rv,rv); > + NS_ADDREF(*aFile = new nsDOMFileFile(localFile)); > + return NS_OK; > + } you don't return anything if AndroidBridge::Bridge() fails ::: mobile/android/app/mobile.js @@ +679,5 @@ > pref("device.camera.enabled", true); > pref("media.realtime_decoder.enabled", true); > > +// enable mozGetUserMedia > +pref("media.enabled", true); I feel this pref name is a bit too generic...

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #50) > content/media/MediaEngineDefault.cpp doesn't seem to be in the current > version of m-c. In which patch is it added? Bug 752351. > You should do: > method() > { > #ifndef ANDROID > *aFile = nsnull; > return NS_ERROR_NOT_IMPLEMNTED; > #else > // Show a file picker with filter being "image/*". > #endif > } > > To show the filepicker, see: AsyncClickHandler::Run() in > nsHTMLInputElement.cpp. > Showing a generic filepicker instead or directly calling the Android backend > method will have the same behavior on Android but will prevent you from > including Android specific stuff and will make possible for other platforms > to implement the same feature easily. This looks a lot cleaner. However, I propose that we do this when we have support for atleast one other platform. Adding the generic file picker at this point will complicate the patch a bit. > BTW, you are aware that this will show an intent-like dialog with the list > of all apps able to pick images (like the Gallery app) *and* all apps able > to take a picture. Yes, the intent is for getUserMedia to let the user choose any source: webcam, file, etc. > ::: dom/base/Navigator.cpp > @@ +915,1 @@ > > if (!Preferences::GetBool("media.enabled", false)) { > > Unless there is a strong reason to do that, you should instead set > "media.enabled" to true by default for Native Fennec in > mobile/android/app/mobile.js I've removed this portion of the patch. In bug 752353, we'll be adding a configure flag and #ifdef to make sure we don't put the API in navigator at all for platforms that don't support it; in addition to a preference.

Comment on attachment 629400 [details] [diff] [review] Android support for still picture capture v4 Nits: Please remove the spurious whitespace changes. The first #ifdef probably should also be MOZ_WIDGET_ANDROID.

Fix whitespace changes and #ifdef

Bah, patch queue fail. This one is real!

Enabled MOZ_MEDIA_NAVIGATOR on Android by default. $ fortune -s Why is that wrong numbers are never busy?

Comment on attachment 629523 [details] [diff] [review] Android support for still picture capture v4.3 We were hoping to land this in time for the Aurora merge, and it's a pretty small patch. Re-assigning to Fabrice for review.

Comment on attachment 629523 [details] [diff] [review] Android support for still picture capture v4.3 Review of attachment 629523 [details] [diff] [review]: ----------------------------------------------------------------- This patch isn't solving some issues Jonas pointed like: - there is no popup blocker-like protection to prevent hijacking the user; - the implementation has strictly no difference compared to <input type='file' accept='image/*'> In addition, the implementation of ::Snapshot() seems incorrect because the user will see an intent-like UI showing any app able to TAKE or PICK a picture. This is going to include the stock Gallery app. I doubt this is what the feature aims to do. ::: content/media/MediaEngineDefault.cpp @@ +169,5 @@ > + > + nsCOMPtr<nsILocalFile> localFile; > + nsresult rv = NS_NewLocalFile(filePath, false, getter_AddRefs(localFile)); > + if (NS_FAILED(rv)) { > + return rv; NS_ENSURE_SUCCESS(rv, rv);

Comment on attachment 629523 [details] [diff] [review] Android support for still picture capture v4.3 Patch backed out from m-i because it was pushed without a proper peer review and rushed out without taking into consideration all comments: https://hg.mozilla.org/integration/mozilla-inbound/rev/1fa953e7d4c1

I do not understand why you guys are rushing this feature even if there are known issues. Rushing a buggy feature to a release seems to be the worst thing you can do to have it widely used.

Comment on attachment 629523 [details] [diff] [review] Android support for still picture capture v4.3 (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #60) > This patch isn't solving some issues Jonas pointed like: > - there is no popup blocker-like protection to prevent hijacking the user; Is this strictly necessary? Do we also use pop-blocker protection for the Geolocation API, for example? In this case, all of navigator.mozGetUserMedia() must be protected, which we should do as a followup to bug 752353. Personally, I feel calls made to mozGetUserMedia() must not be popup-blocked as there are valid use-cases for developers to make a call to that function without user interaction with the web page. > - the implementation has strictly no difference compared to <input > type='file' accept='image/*'> This has been discussed extensively in the past and we have concluded that we need a programatic API to capture pictures in addition to a declarative API. See: https://groups.google.com/d/msg/mozilla.dev.media/z3Pxghxmfno/2Oeh0jGl39wJ > In addition, the implementation of ::Snapshot() seems incorrect because the > user will see an intent-like UI showing any app able to TAKE or PICK a > picture. This is going to include the stock Gallery app. I doubt this is > what the feature aims to do. I pointed this out before, this is exactly what the feature aims to do. getUserMedia should let the user pick any local source for media *in addition* to hardware. In consideration of the above, I'd like to request a re-review of the same code.

Comment on attachment 629523 [details] [diff] [review] Android support for still picture capture v4.3 Review of attachment 629523 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Anant Narayanan [:anant] from comment #63) > Comment on attachment 629523 [details] [diff] [review] > Android support for still picture capture v4.3 > > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #60) > > This patch isn't solving some issues Jonas pointed like: > > - there is no popup blocker-like protection to prevent hijacking the user; > > Is this strictly necessary? Do we also use pop-blocker protection for the > Geolocation API, for example? I don't see how this is related. > In this case, all of > navigator.mozGetUserMedia() must be protected, which we should do as a > followup to bug 752353. Maybe not all of .mozGetUserMedia(). > Personally, I feel calls made to mozGetUserMedia() must not be popup-blocked > as there are valid use-cases for developers to make a call to that function > without user interaction with the web page. Imagine that scenario: a user plays a wack-a-mole game from a malicious web page. At some point, when a mole appears, the game will call .Snapshot() and the capture UI will show up just before the user press the screen. If the mole was well positioned, it would be exactly on the capture button and the developer will have been able to still an image from the user's device. This said, the method you are calling will always show an UI between the capture UI and the first click. So, this will make the hi-jack harder. However, the popup blocker protection isn't hard to implement and would be safer in case of future changes. BTW, using nsIFilePicker instead of directly calling the Android method would allow you to write tests for this feature given that we have a mock implementation of nsIFilePicker in our test suite. IMO, you should use that. Anyhow, I believe Jonas should review that. He knows more about the context than I do.

mounir, this wack-a-mole game... can't you do that right now with the input element and .click()?

(In reply to Doug Turner (:dougt) from comment #65) > mounir, this wack-a-mole game... can't you do that right now with the input > element and .click()? You need a user interaction to be able to call .click(). It is allowed to call it from a setTimeout() if the setTimeout() is generated from a user interaction. Unfortunately, it is allowed to pass a time <= 1s to the setTimeout. So, indeed, it could be doable. However, this is fixable (maybe) and it is a bit harder.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #64) > Comment on attachment 629523 [details] [diff] [review] > Android support for still picture capture v4.3 > > Review of attachment 629523 [details] [diff] [review]: > ----------------------------------------------------------------- > > (In reply to Anant Narayanan [:anant] from comment #63) > > Comment on attachment 629523 [details] [diff] [review] > > Android support for still picture capture v4.3 > > > > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #60) > > > This patch isn't solving some issues Jonas pointed like: > > > - there is no popup blocker-like protection to prevent hijacking the user; > > > > Is this strictly necessary? Do we also use pop-blocker protection for the > > Geolocation API, for example? > > I don't see how this is related. navigator.getUserMedia and navigator.geolocation are very similar APIs from a permission point of view. Both result in a doorhanger prompt, but for the {picture:true} case in getUserMedia we show an intent dialog on Android only. This would still be a doorhanger/preview widnow on Desktop, for example. > > In this case, all of > > navigator.mozGetUserMedia() must be protected, which we should do as a > > followup to bug 752353. > > Maybe not all of .mozGetUserMedia(). Right, I think we may want popup blocking only for the picture case, and not video/audio too. Definitely needs more discussion, but I think it should be acceptable for us to do exactly what <input> does (does it have popup blocking as well?). Also, I'd point out that the goal isn't to aim for "mass adoption" of the API, getUserMedia is still an early draft. We do want early feedback from developers, especially the ones who plan to submit camera-based apps to our marketplace (one of the reasons we want this in Fx15), and one of the ways to do that is to ship it.

This feature is needed for B2G (and is being targeted for B2G v1), and we were/are trying to keep some parity between Android and B2G. Desktop support for this is almost done and will be landing shortly. In particular, this feature is needed in Android for Kilimanjaro and for apps in the Mozilla Marketplace, which is going to Beta soon. I think the Marketplace is planning to go Beta with Firefox 15 which is why having this in Firefox 15 would give app devs something to play with and give feedback on. We realize <input> is available, but getting feedback on getUserMedia (which is what we're going to be asking devs to use when we launch) is what's really useful to us. Our discussion went all the way up to Brendan. There were emails and hallway conversations, and ultimately Brendan endorsed our doing this. See the thread that Anant references above. What's more, Mozilla is trying to push forward the definition of getUserMedia in the W3C, and nothing pushes a spec forward more effectively than implemented code that devs have used and have given feedback on. We did have a discussion with jst about all of this on Thursday (we also talked to him about B2G and 3 other bugs that are part of this work) before trying to land, and he thought it was worth trying. We spent a bunch of our time on Thursday, Friday and part of Saturday (with much help from jst and dougt) landing the other bugs (bug 752353, bug 750943, and bug 752351), and we saw this one as the last one in the "getUserMedia phase 1" group worth trying for Firefox 15 (since B2G runs completely separately from the trains). We tried really hard to find an Android peer on Saturday afternoon and especially all day Sunday, and no one was available. All of us who were online during that time had convinced ourselves that we had addressed all the blocker issues and that an Android reviewer would bless the patch once he/she was available (it was such a tiny patch), and we thought we were helping the Marketplace apps and the forward progress of getUserMedia. We do want to offer the gallery as a choice, and we can talk more about why if that's a real concern. Fundamentally, we're trying to get this API into app devs hands for feedback, and we'll get more eyeballs (and thus better feedback) if we can get this into Firefox 15.

As mentioned, it would be very hard in this implementation (or <input>) to make the whack-a-mole game attack work, as it would pop up an intent list, and then there'd be a delay to pop the actual app, and then you'd have to get them to click in the right place in the app, etc. It is more of an argument for the raw snapshot if the backend gave a direct snapshot without intents or an app, though an alternative solution to that issue is to leverage the security discussions and permissions model being worked out, so that "installed" apps given access to camera functions could avoid a blocker or extra clicks or doorhangers. But that wouldn't apply to this patch.

Maire, As much as I'd like to see that landed, arbitrary decided timelines should not take precedence on shipping correct code. I did r+ yesterday because I trusted people to have addressed all previous comments.

+1

(In reply to Fabrice Desré [:fabrice] from comment #70) > Maire, > > As much as I'd like to see that landed, arbitrary decided timelines should > not take precedence on shipping correct code. Please know that was never the intention here. We all thought we were asking to ship correct code yesterday. Mounir asked this morning in this bug "why do we care?" (and why we pushed so hard to get all the concerns addressed in time for Firefox 15 since <input> is already there). I was attempting to answer that since the thread Anant referenced, while correct, is long and can be hard to follow in places. I was trying to help by summarizing and give some additional background. > I did r+ yesterday because I trusted people to have addressed all previous > comments. I know, and I really appreciated your stepping up and trying to do what we all thought was the right thing. Everyone I talked to yesterday thought we were good to go, and I *think* what we're discussing now is whether we truly are good to go or whether the popup blocking code is essential. That is why we're asking for a review and judgement call from Jonas. If it's not ready to go in, it's not ready.

Backed out: https://hg.mozilla.org/mozilla-central/rev/1fa953e7d4c1

(In reply to Fabrice Desré [:fabrice] from comment #70) > As much as I'd like to see that landed, arbitrary decided timelines should > not take precedence on shipping correct code. I agree. But, what we are really discussing here is that whether the code is correct. It seems like we have a variety of opinions on whether the code is correct or not :) > I did r+ yesterday because I trusted people to have addressed all previous > comments. And we did, faithfully and to the best of our abilities. The popup behavior in this patch is not significantly different than that of <input>.

While the popup blocker issue might not be significant here from a malicious whackamole game point of view I do feel we should add that in (and I wish I had thought about that in reviewing the API addition in the first place) so that we don't allow use of this API in cases where we might not in future versions. It's always easier to loosen our policies at a later time than to tighten them up. Experimental API or not. It sounds like using the nsIFilePicker API here would be good from a testing point of view, but it also sounds like that does not give the behavior we want here, so I suspect we need to think more about that going forward (i.e. in another bug). If we can get agreement on that soon, I'd be in favor of still taking this on aurora (not saying that will happen, but I'd support trying). If not, we need to disable this whole API in 15 due to lack of backends.

Comment on attachment 629523 [details] [diff] [review] Android support for still picture capture v4.3 minus patch.

(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #76) > It sounds like using the nsIFilePicker API here would be good from a testing > point of view, but it also sounds like that does not give the behavior we > want here, so I suspect we need to think more about that going forward (i.e. > in another bug). The current patch is exactly similar than using <input type='file' accept='image/*'> so using nsIFilePicker would have the same behavior. It might not be the case in the long term though.

Added popup blocking. We're querying the nsIPopupWindowManager, is there something else we should be doing in addition to this, or does the PopupWindowManager automatically know that this request was a result of user-action?

Comment on attachment 629995 [details] [diff] [review] Android support for still picture capture v5 Review of attachment 629995 [details] [diff] [review]: ----------------------------------------------------------------- I would prefer if Jonas can review that. There is no need to sr here. ::: dom/media/MediaManager.cpp @@ +193,5 @@ > + > + PRUint32 permission; > + nsCOMPtr<nsIDocument> doc = mWindow->GetExtantDoc(); > + pm->TestPermission(doc->GetDocumentURI(), &permission); > + if (permission == nsIPopupWindowManager::DENY_POPUP) { Hmm, you should call: GetPopupControlState() (on nsPIDOMWindow) and check if the returned PopupControlState is greater than openControlled before checking the permission. See content/html/content/src/nsHTMLInputElement.cpp AsyncClickHandler::Run() (around line 230).

Check for nsPIDOMWindow::GetPopupControlState.

Comment on attachment 630186 [details] [diff] [review] Android support for still picture capture v5.1 Review of attachment 630186 [details] [diff] [review]: ----------------------------------------------------------------- Admittedly, I don't understand a lot of what's going on here, so you should probably get a review from someone that know the MedaManager better as well. But r=me if you do that as well.

Comment on attachment 630186 [details] [diff] [review] Android support for still picture capture v5.1 roc asked me to review the MediaManager part

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/b6c5485ab236 The build was busted: https://tbpl.mozilla.org/php/getParsedLog.php?id=12504468&tree=Mozilla-Inbound, investigating.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ab13a310a644

Whoah! All instances of "nsILocalFile" were replaced to "nsIFile" on June 6th: http://hg.mozilla.org/mozilla-central/rev/a15d75939cd5 So, despite what you might think about a function named "NS_NewLocalFile", it takes an nsIFile and not an nsILocalFile. Unfortunately, the last time I built this patch was when it was submitted, i.e. on June 5th. Busted! I got a local build to succeed, checking on try again: https://tbpl.mozilla.org/?tree=Try&rev=ee14b59d599d

Comment on attachment 631585 [details] [diff] [review] Android support for still picture capture v5.2 Review of attachment 631585 [details] [diff] [review]: ----------------------------------------------------------------- Diffed the diffs, very simple. Straightforward bustage fix. Try build shows all android builds clean, and normal random pattern in the tests.

re-landed with green try https://hg.mozilla.org/integration/mozilla-inbound/rev/2304b82d86e4

https://hg.mozilla.org/mozilla-central/rev/2304b82d86e4 Per the tree rules, please set the target milestone when landing on inbound.

Comment on attachment 631585 [details] [diff] [review] Android support for still picture capture v5.2 [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 738528 had landed just before uplift, but was immediately backed out to add popup blocking. User impact if declined: The other related patches to provide the rest of the APIs (DOM, etc) all landed, but without a working useful backend, we'll need to pull them. So we must uplift this patch or back out all the others. This feature is very much wanted for the Marketplace launch and to help drive discussion/adoption of the getUserMedia still image API in the W3C. We realize <input> is available, but getting feedback on getUserMedia (which is what we're going to be asking devs to use when we launch the Marketplace -- and what we're trying to standardize in the W3X) is what's truly useful to us. Testing completed: Landed on m-c. Testing image capture requires manual testing at this time. Risk of taking this patch: This patch only affects Android. The backend code included here invokes the same intent popup that <input type='file'> for images does. Primary risk would be if there were a problem in the popup-blocking code. This code is only accessible via navigator.mozGetUserMedia(), so the risk of breaking existing pages/apps is very low. String or UUID changes made by this patch: none

Comment on attachment 631585 [details] [diff] [review] Android support for still picture capture v5.2 Thank you for the risk vs reward evaluation below. (In reply to Randell Jesup [:jesup] from comment #91) > User impact if declined: The other related patches to provide the rest of > the APIs (DOM, etc) all landed, but without a working useful backend, we'll > need to pull them. So we must uplift this patch or back out all the others. > This feature is very much wanted for the Marketplace launch and to help > drive discussion/adoption of the getUserMedia still image API in the W3C. > We realize <input> is available, but getting feedback on getUserMedia (which > is what we're going to be asking devs to use when we launch the Marketplace > -- and what we're trying to standardize in the W3X) is what's truly useful > to us. > Risk of taking this patch: This patch only affects Android. The backend > code included here invokes the same intent popup that <input type='file'> > for images does. Primary risk would be if there were a problem in the > popup-blocking code. This code is only accessible via > navigator.mozGetUserMedia(), so the risk of breaking existing pages/apps is > very low. Given: * how early it is in the cycle * the fact that this only introduces risk to Android * the fact that existing pages/apps are very unlikely to regress We're approving for Aurora 15.

checkin-needed for mozilla-aurora

Funny story - the Aurora merge happened prior to comment 87, meaning that version 5.2 was red. Backed out. https://hg.mozilla.org/releases/mozilla-aurora/rev/86679834ebc9 I assume that version 5.1 should be the patch landing on Aurora?

Comment on attachment 630186 [details] [diff] [review] Android support for still picture capture v5.1 [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 749930 (nsIFile API change which landed right after Aurora uplift). As ryan figured out, approval was asked for the wrong patch; I didn't realize the reason Anant had to revise the patch before (for m-c re-landing) was a post-uplift API change for nsIFile/nsILocalFile. User impact if declined: Must back out the rest of the getUserMedia patches Testing completed (on m-c, etc.): on m-c. Testing requires running on a android device. https://tbpl.mozilla.org/?tree=Try&rev=c9f75db7b1b0 passes green (mochitests being busted I'm told is some type of branding issue unrelated to this bug, and I pushed a "clean" aurora try to verify: https://tbpl.mozilla.org/?tree=Try&rev=6f48a4c556e3). Risk to taking this patch (and alternatives if risky): Minimal risk String or UUID changes made by this patch: none.

The oranges for android mochitests/etc are bug 725703, and unrelated to this patch

Check-in needed into Aurora for attachment 630186 [details] [diff] [review].

Note: there will be a merge conflict between patches v5.1 and v5.2 during the next Aurora uplift. I'm assuming whoever does the merge is used to seeing them, it can be resolved manually with little effort.

(In reply to Anant Narayanan [:anant] from comment #98) > Check-in needed into Aurora for attachment 630186 [details] [diff] [review]. https://hg.mozilla.org/releases/mozilla-aurora/rev/14cb9ba83ac3

(In reply to Randell Jesup [:jesup] from comment #96) > Comment on attachment 630186 [details] [diff] [review] > Android support for still picture capture v5.1 > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): Bug 749930 (nsIFile API change > which landed right after Aurora uplift). As ryan figured out, approval was > asked for the wrong patch; I didn't realize the reason Anant had to revise > the patch before (for m-c re-landing) was a post-uplift API change for > nsIFile/nsILocalFile. > > User impact if declined: Must back out the rest of the getUserMedia patches > > Testing completed (on m-c, etc.): on m-c. Testing requires running on a > android device. Can QA get some content to manually test against? Are there sample apps or websites that will exercise the use cases that this patch touches? I found a testcase in https://bug765552.bugzilla.mozilla.org/attachment.cgi?id=638085 that we can test against android Firefox. But any additional help for QA is requested. > > Risk to taking this patch (and alternatives if risky): Minimal risk > > String or UUID changes made by this patch: none.

(In reply to Tony Chung [:tchung] from comment #101) > Can QA get some content to manually test against? Are there sample apps or > websites that will exercise the use cases that this patch touches? > > I found a testcase in > https://bug765552.bugzilla.mozilla.org/attachment.cgi?id=638085 that we can > test against android Firefox. But any additional help for QA is requested. I made a simple test page: https://people.mozilla.com/~anarayanan/gum_test.html On Android, only the picture option will work. Expected behaviour is a still image from the device's camera appearing on the screen when the "Picture" button is clicked. I'm not aware of any "real" apps of websites that use this feature yet as it is still very new. If I can help by writing up more example pages, let me know!

(In reply to Anant Narayanan [:anant] from comment #102) > (In reply to Tony Chung [:tchung] from comment #101) > > Can QA get some content to manually test against? Are there sample apps or > > websites that will exercise the use cases that this patch touches? > > > > I found a testcase in > > https://bug765552.bugzilla.mozilla.org/attachment.cgi?id=638085 that we can > > test against android Firefox. But any additional help for QA is requested. > > I made a simple test page: > https://people.mozilla.com/~anarayanan/gum_test.html > > On Android, only the picture option will work. Expected behaviour is a still > image from the device's camera appearing on the screen when the "Picture" > button is clicked. > > I'm not aware of any "real" apps of websites that use this feature yet as it > is still very new. If I can help by writing up more example pages, let me > know! Might be worth btw following up with Joe Stagner on this - more dogfooding on this API would definitely help get some initial feedback on what developers think of this API.

(In reply to Anant Narayanan [:anant] from comment #102)al help for QA is requested. > I made a simple test page: > https://people.mozilla.com/~anarayanan/gum_test.html > > On Android, only the picture option will work. Expected behaviour is a still > image from the device's camera appearing on the screen when the "Picture" > button is clicked. > Tested against the 7-6-2012 nightly build. "Picture" opened up camera activity, took a picture, and saw it get saved to the test webpage. You have to add media.navigator.enabled=true to config settings first. Dont forget to restart fennec. per IRC with anant, audio and video is set to land in FF16 desktop first, and not android. tchung: anant: woo! thanks, that worked [3:43pm] anant: tchung: awesome! in ff16 we will try and get that pref enabled by default - just wanted some early testing before doing that [3:43pm] tchung: what about the other two [3:43pm] tchung: video and audio [3:43pm] tchung: is gUM set for those types yet [3:43pm] anant: we're working on that for desktop in ff16, no eta on android yet [3:44pm] anant: so desktop should have all three in ff16, android will only have picture until we decide when to tackle that implementation

(In reply to Tony Chung [:tchung] from comment #104) > (In reply to Anant Narayanan [:anant] from comment #102)al help for QA is > requested. > > I made a simple test page: > > https://people.mozilla.com/~anarayanan/gum_test.html > > > > On Android, only the picture option will work. Expected behaviour is a still > > image from the device's camera appearing on the screen when the "Picture" > > button is clicked. > > > > Tested against the 7-6-2012 nightly build. "Picture" opened up camera > activity, took a picture, and saw it get saved to the test webpage. > > You have to add media.navigator.enabled=true to config settings first. Dont > forget to restart fennec. > > per IRC with anant, audio and video is set to land in FF16 desktop first, > and not android. > > tchung: anant: woo! thanks, that worked > [3:43pm] anant: tchung: awesome! in ff16 we will try and get that pref > enabled by default - just wanted some early testing before doing that > [3:43pm] tchung: what about the other two > [3:43pm] tchung: video and audio > [3:43pm] tchung: is gUM set for those types yet > [3:43pm] anant: we're working on that for desktop in ff16, no eta on android > yet > [3:44pm] anant: so desktop should have all three in ff16, android will only > have picture until we decide when to tackle that implementation Tony - Is the verification complete for this then? If so, can you mark this as verified?

(In reply to Jason Smith [:jsmith] from comment #105) > (In reply to Tony Chung [:tchung] from comment #104) > > (In reply to Anant Narayanan [:anant] from comment #102)al help for QA is > > requested. > > > I made a simple test page: > > > https://people.mozilla.com/~anarayanan/gum_test.html > > > > > > On Android, only the picture option will work. Expected behaviour is a still > > > image from the device's camera appearing on the screen when the "Picture" > > > button is clicked. > > > > > > > Tested against the 7-6-2012 nightly build. "Picture" opened up camera > > activity, took a picture, and saw it get saved to the test webpage. > > > > You have to add media.navigator.enabled=true to config settings first. Dont > > forget to restart fennec. > > > > per IRC with anant, audio and video is set to land in FF16 desktop first, > > and not android. > > > > tchung: anant: woo! thanks, that worked > > [3:43pm] anant: tchung: awesome! in ff16 we will try and get that pref > > enabled by default - just wanted some early testing before doing that > > [3:43pm] tchung: what about the other two > > [3:43pm] tchung: video and audio > > [3:43pm] tchung: is gUM set for those types yet > > [3:43pm] anant: we're working on that for desktop in ff16, no eta on android > > yet > > [3:44pm] anant: so desktop should have all three in ff16, android will only > > have picture until we decide when to tackle that implementation > > Tony - Is the verification complete for this then? If so, can you mark this > as verified? yes, still image support on android is verified.

it looks like bug 773847 has been filed as gUM regressed when it landed for desktop. we'll track the work in that bug.