Currently the video recorder defaults to 352x288. Need to expose other profiles to the apps.

Should this block bug #795330?

The .recorderProfiles object exposes a collection of objects, one for each profile and named for the specific supported profile types. Each of these subobjects exposes a set of properties that describe the profile. Audio and video properties are grouped into subobjects. e.g. .recorderProfiles .low .format (e.g. "3gp") -- useful as a file extension .audio .channels (e.g. 1) .bitrate (bits per second) .samplerate .codec (descriptive text, e.g. "amrnb") .video .codec (descriptive text, e.g. "h263", "h264", "mpeg4sp") .bitrate (bits per second) .framerate (e.g. 30) .width (e.g. 176) .height (e.g. 144) .1080p .format (e.g. "3gp") -- useful as a file extension .audio .channels (e.g. 2) .bitrate .samplerate .codec (e.g. "aac") .video .codec (e.g. "h264") .bitrate .framerate .width (e.g. 1920) .height (e.g. 1080) The intent is that the profile name can be passed to startRecording(), which will configure the camera and encoder. The remaining properties are solely for the application's inspection. Sample patch for camera.js forthcoming.

Comment on attachment 671580 [details] [diff] [review] Expose CameraCapabilities.recorderProfiles - v1 This looks good to me! The one comment that I have is that I'd prefer the GetJsObject() method implementations to not be inline in the headers to save on code size if these get called in more than a couple of places. That is not the case in this patch itself, so I can deal, but if this spreads, I think they belong in a .cpp file instead of in the header.

(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #4) > Comment on attachment 671580 [details] [diff] [review] > Expose CameraCapabilities.recorderProfiles - v1 > > This looks good to me! > > The one comment that I have is that I'd prefer the GetJsObject() method > implementations to not be inline in the headers to save on code size if > these get called in more than a couple of places. That is not the case in > this patch itself, so I can deal, but if this spreads, I think they belong > in a .cpp file instead of in the header. I agree. Will leave it as-is for now, but promise to break this out into a .cpp if it winds up growing. Thanks! try server push: https://tbpl.mozilla.org/?tree=Try&rev=51974ecdd2a0

try server push: https://tbpl.mozilla.org/?tree=Try&rev=32bde0607c81

Its almost a month past feature freeze and this is very clearly not bugfixing work. Are we sure we need this feature?

Comment on attachment 671580 [details] [diff] [review] Expose CameraCapabilities.recorderProfiles - v1 Review of attachment 671580 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me in case we decide we need this for v1.

Newer version shows how to get and interpret .recorderProfiles, and includes some code to test changes to .videoSizes. NOT FOR CHECKIN. Just to illustrate using the API.

Hi Johnny, sorry about the re-request for re-review, but while testing the next phase of the video recorder changes (in parallel with trying to get this patch landed) I found a couple of deficiencies here. This revision includes a couple of extra changes over the previous one: 1. A new CameraRecorderProfiles method, SetSupportedResolutions(), which is used to set a filter on the profiles to return only those actually supported by the underlying camera hardware. To support this, I added GetVideoSizes() to the camera control interface, and this is now also fed into DOMCameraCapabilities.cpp. 2. With B2G_DEBUG=1, MOZ_ASSERT() triggers a SIGSEGV, and I started catching a threading failure in the destructor for nsCOMPtr<nsIDOMDeviceStorage> in CameraControlImpl.h::class StartRecordingTask. Fixed that--see CameraControlImpl.cpp::StartRecording() and CameraControlImpl.h::class StartRecordingResult.

Apologies. This updated patch adds more diff context, a proper header, and fixes a non-B2G breakage. try server push: https://tbpl.mozilla.org/?tree=Try&rev=642da21c252e

try server push: https://tbpl.mozilla.org/?tree=Try&rev=e6d9bfb3b35e

Comment on attachment 673769 [details] [diff] [review] Expose CameraCapabilities.recorderProfiles - v4 jst, do you have time to review this? The patch now includes support for passing a video profile option to the recorder, and some related changes to the code you had already reviewed.

A small tweak to GonkRecorderProfiles.def to remove some recorder profiles that are causing problems.

try server results: https://tbpl.mozilla.org/?tree=Try&rev=af01124cab0f

Comment on attachment 674025 [details] [diff] [review] Expose CameraCapabilities.recorderProfiles - v5 - In CameraControlImpl::StartRecording(): + NS_ENSURE_TRUE(aStorageArea, NS_ERROR_INVALID_ARG); + + nsCOMPtr<nsISupports> ref = do_QueryInterface(aStorageArea); + NS_ENSURE_TRUE(ref, NS_ERROR_INVALID_ARG); + NS_ADDREF(ref); Why not just call NS_ADDREF(aStorageArea) here instead of QI'ing to nsISupports and then incrementing the reference count again through the nsCOMPtr, only to let the nsCOMPtr fall out of scope and decrement the refcount? Also, by XPCOM rules you don't need to null check ref there since you've already null checked aStorageArea, and QI to nsISupports is not permitted to fail. - In StartRecordingResult::Run(): + nsCOMPtr<nsISupports> ref = do_QueryInterface(mStorageArea); + NS_IF_RELEASE(ref); Same here, just use NS_IF_RELEASE(mStorageArea). Actually, looking a bit deeper here, it seems the only reason you need to even pass the storage area around here is so that nsGonkCameraControl::StartRecordingImpl() can call GetRootDirectory() on it. Rather than doing this manual refcounting dance, could you not simply get the root directory out of the storage area and pass a clone of that in the runners? That would let you get rid of the not threadsafe (but possibly ok in practice) call to GetRootDirectory() too :). And as long as you pass a clone of a file, everything should be thread safe for you. r- until that's fixed, but fixing that should be very straight forward, nsCOMPtr's all over for the nsIFile stuff etc. The rest looks good!

Incorporating your feedback. This makes things so much simpler!

try server push: https://tbpl.mozilla.org/?tree=Try&rev=4ece096b6a92

Comment on attachment 674105 [details] [diff] [review] Expose CameraCapabilities.recorderProfiles - v6 - In CameraControlImpl::StartRecording(): + nsCOMPtr<nsIRunnable> startRecordingTask = new StartRecordingTask(this, *aOptions, aFolder, aFilename, onSuccess, onError, mWindowId); return mCameraThread->Dispatch(startRecordingTask, NS_DISPATCH_NORMAL); Throw in: nsCOMPtr<nsIFile> clone; aFolder->Clone(getter_AddRefs(clone)); before that and pass "clone" instead of aFolder. That way you're guaranteed that the folder won't change between now and StartRecordingTask::Run() runs etc. r=jst with that!

Incorporating latest feedback from :jst. try server push: https://tbpl.mozilla.org/?tree=Try&rev=f5cbb8f36ea5

Comment on attachment 674117 [details] [diff] [review] Expose CameraCapabilities.recorderProfiles - v7 Looks good!

(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #21) > Comment on attachment 674117 [details] [diff] [review] > Expose CameraCapabilities.recorderProfiles - v7 > > Looks good! Thank you for the rapid review cycle!

https://hg.mozilla.org/integration/mozilla-inbound/rev/a882fd383dbb I wait to push patches to Aurora until after they land successfully on m-c, FWIW. However, I have bug queries for finding bb+ bugs that need Aurora landing, so it won't be forgotten about. Also, should this have a test?

(In reply to Ryan VanderMeulen from comment #24) > https://hg.mozilla.org/integration/mozilla-inbound/rev/a882fd383dbb > > Also, should this have a test? Currently almost all of the camera API is stubbed into non-existence for anything other than the b2g builds, and even there it requires working camera hardware to test. Currently I run through tests manually. :( dale, once this lands, you'll need to add a new parameter to the beginning of the startRecording() call.