991962 - [Gonk][Camera][hamachi] Recorded video zoom does not match preview zoom level

Status: RESOLVED INCOMPLETE

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[mkruml]

Attachment: logcat.txt

Description:
If a video is recorded at 100% zoom and then replayed, the user will see that the video was recorded as if no zoom was used.

Repro Steps:
1) Update a Buri to BuildID: 20140403000210
2) Go to Camera > Video
3) Use the zoom feature to zoom in 100% and record a video
4) Navigate to the video gallery and play the recorded video

Actual:
The video was recorded as if zoom was not used

Expected:
The video was recorded as it appears on the screen

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140403000210
Gaia: b4f3b84ec68233a99fd5865c15cfe28aebe26531
Gecko: 5bccd264a0e3
Version: 30.0a2
Firmware Version: v1.2-device.cfg


Notes:

Repro frequency: 100%
See attached logcat: logcat.txt

Comment 1

[mkruml]

Attachment: VIdeo Bug

Video attached: Zoom1.mp4

Comment 2

[Jason Smith [:jsmith]]

Seems like this is busted new feature.

Comment 3

[Justin D'Arcangelo [:justindarc]]

The issue on Buri/Hamachi seems to be that the recorded video is not zoomed in as much as the preview is showing. It is, however, zooming in the recorded video. To verify, start recording a video without zooming at all (1x), then while recording, zoom in the whole way (4x) then back out again to 1x. In the recorded video, you will see the zooming working, but only up to a certain point (maybe 2x?). I'm looking into this further to see if perhaps we need to re-query the hardware's zoom capabilities in video mode.

Comment 4

[Justin D'Arcangelo [:justindarc]]

Mike: This is strange. For some reason, on Hamachi, video recordings cannot be zoomed in beyond ~1.8x. In this case, we're talking about the actual recorded video, *NOT* the preview stream. Any ideas?

Comment 5

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Justin D'Arcangelo [:justindarc] from comment #4)
>
> Mike: This is strange. For some reason, on Hamachi, video recordings cannot
> be zoomed in beyond ~1.8x. In this case, we're talking about the actual
> recorded video, *NOT* the preview stream. Any ideas?

Hamachi doesn't support zoom.

Comment 6

[Justin D'Arcangelo [:justindarc]]

Really? The capabilities.zoomRatios array has values and the pictures are taken with the correct zoom applied.

Comment 7

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Justin D'Arcangelo [:justindarc] from comment #6)
>
> Really? The capabilities.zoomRatios array has values and the pictures are
> taken with the correct zoom applied.

O RLY? Hmm, I'll have to look into that. AFAIK, hamachi doesn't support zoom (or autofocus).

Comment 8

[Hema Koka [:hema]]

Need to confirm if zoom is supported on Hamachi before we decide on the blocker (can't call it broken feature yet)

Comment 9

[Justin D'Arcangelo [:justindarc]]

Picture zoom is definitely working on Hamachi and the `mozCamera.capabilities.zoomRatios` API is returning an array of zoom levels on the device which we interpret as meaning that the hardware *does* support it. Strangely though, video zooming on Hamachi works, but only up to around 1.8x, not the 4.0x that it reports.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Hema Koka [:hema] from comment #8)
> Need to confirm if zoom is supported on Hamachi before we decide on the
> blocker (can't call it broken feature yet)

Note - if we don't support zoom, then that means we should not be allowing the user to zoom in with the camera. Otherwise, we'll confuse the user since the UX is indicating that we do support this feature, even though the implementation indicates it's not supported. So I'd argue that this is still a blocker if we're exposing zoom when it's not intended to work, as that's confusing UX.

Comment 11

[Justin D'Arcangelo [:justindarc]]

I agree with Jason's comment 10. However, what I meant was, the `mozCamera.capabilities.zoomRatios` API is how we detect if zoom is available or not. And on the Hamachi, it is returning values that appear to be valid for taking photos, but not videos. We have two options here:

1.) Fix or make an exception in the Gecko API for Hamachi
2.) Add a build-time configurable option to disable zoom on Hamachi

Comment 12

[Hema Koka [:hema]]

(In reply to Jason Smith [:jsmith] from comment #10)
> (In reply to Hema Koka [:hema] from comment #8)
> > Need to confirm if zoom is supported on Hamachi before we decide on the
> > blocker (can't call it broken feature yet)
> 
> Note - if we don't support zoom, then that means we should not be allowing
> the user to zoom in with the camera. Otherwise, we'll confuse the user since
> the UX is indicating that we do support this feature, even though the
> implementation indicates it's not supported. So I'd argue that this is still
> a blocker if we're exposing zoom when it's not intended to work, as that's
> confusing UX.

yes we are on the same page...

however it is not clear whether it is supported or not...let mikeh come back with input on this

if this is not supported in hamachi, we can disable it for that device.

(zoom is a required feature for madai device)

thanks
hema

Comment 13

[Mike Habicher [:mikeh] (high bugzilla latency)]

I can confirm that this definitely happens. It's not at all clear why.

I'm prepping a build that may shine some light on what the camera driver thinks is going on. Will update this bug once I know something.

Comment 14

[Mike Habicher [:mikeh] (high bugzilla latency)]

Ah, okay--it seems we're resorting to CSS transforms to present the zoomed-in viewfinder in the case where the camera can't provide us with zoomed-in frames properly. This doesn't affect picture taking because that process is something separate, whereas recording happens on the same buffers that go to the viewfinder, except that the CSS transform only applies to the viewfinder. So the frames are recorded un-zoomed.

Unfortunately, as with all devices we've tested, KEY_MAX_ZOOM doesn't reflect the zoomable limit of the camera based on preview resolution. Rather, it always returns the same value: 59, the number of zoom levels supported.

So, in short:
- hamachi supports zoom
- we can't support arbitrary zoom when video recording

Comment 15

[Tony Chung [:tchung]]

given hamachi supports zoom, and testing is being done it, we should restore the zoom level properly.  1.4+

Comment 16

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Tony Chung [:tchung] from comment #15)
>
> given hamachi supports zoom, and testing is being done it, we should restore
> the zoom level properly.  1.4+

Tony, I think this comment belongs in bug 993043.

This bug is about the recording zoom not matching the zoom shown on the preview.

Comment 17

[Hema Koka [:hema]]

Chris, 

Since Sri is out, we need product input here...

Is it acceptable for picture and video modes to have different zoom limits?

Thanks
Hema

Comment 18

[Mike Habicher [:mikeh] (high bugzilla latency)]

I wonder if this could be due to our non-implementation of NATIVE_WINDOW_SET_SCALING_MODE and NATIVE_WINDOW_SET_CROP in http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkNativeWindowClientICS.cpp#256 .

Comment 19

[Mike Habicher [:mikeh] (high bugzilla latency)]

I see that these settings _are_ supported in JB[1] and KK[2], which could explain why zooming while recording works properly on (e.g.) the Nexus 4.

1. http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkNativeWindowClientJB.cpp#372
2. http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkNativeWindowClientKK.cpp#359

Comment 20

[Mike Habicher [:mikeh] (high bugzilla latency)]

It looks like _SCALING_MODE and _CROP are supported in the AOSP ICS/4.0.4 SurfaceTextureClient[1], upon which (I believe) GonkNativeWindowClientICS.cpp is based.

1. http://androidxref.com/4.0.4/xref/frameworks/base/libs/gui/SurfaceTextureClient.cpp#288

Sotaro, what do you think?

Comment 21

[Mike Habicher [:mikeh] (high bugzilla latency)]

I've been looking into what's involved in implementing _SCALING_MODE and _CROP (and _TRANSFORM) in GonkNativeWindowClientICS.cpp/.h, and it's non-trivial. e.g. in the AOSP version of setCrop(), there is a call to mSurfaceTexture->setCrop()[1]. This maps cleanly to a call to mNativeWindow->setCrop() (similar to [2]), but our current implementation of GonkNativeWindowICS::setCrop() currently just returns OK[3]. setTransform() and setScalingMode() do the same.

In AOSP ICS, these methods set local members[4] which are then applied when the new buffer is queued-up in queueBuffer()[5], which is completely different from how it is implemented in JB and KK.

1. http://androidxref.com/4.0.4/xref/frameworks/base/libs/gui/SurfaceTextureClient.cpp#452
2. http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkNativeWindowClientICS.cpp#392
3. http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkNativeWindowICS.cpp#570
4. http://androidxref.com/4.0.4/xref/frameworks/base/libs/gui/SurfaceTexture.cpp#637
5. http://androidxref.com/4.0.4/xref/frameworks/base/libs/gui/SurfaceTexture.cpp#593

Eventually the values make their way to SurfaceTexture::getCurrentCrop(), getCurrentTransform(), and getCurrentScalingMode(), which seem to only get used in Layers.cpp in surfaceflinger (which we don't use in b2g).

Comment 22

[Sotaro Ikeda [:sotaro]]

(In reply to Mike Habicher [:mikeh] from comment #20)
> It looks like _SCALING_MODE and _CROP are supported in the AOSP ICS/4.0.4
> SurfaceTextureClient[1], upon which (I believe)
> GonkNativeWindowClientICS.cpp is based.
> 
> 1.
> http://androidxref.com/4.0.4/xref/frameworks/base/libs/gui/
> SurfaceTextureClient.cpp#288
> 
> Sotaro, what do you think?

In android, camera preview zoom is handled by SurfaceTexture. In b2g, camera preview zoom is handled by camera app. In android case, zoom preview is directly controlled by camera hal via SurfaceTexture. But in b2g, camera hal does not control camera preview zooming.

_SCALING_MODE and _CROP is not handled in b2g. Between b2g and android, architecture is very different. It is very difficult to add full support of SCALING_MODE and _CROP. In android, SurfaceTexutre is almost same to rendering target. But in b2g, camera preview frame can be used for anything within media framework.

Use _SCALING_MODE and _CROP in GonkNativeWindow means that stopping camera preview zoom control in camera app side.

Comment 23

[Sotaro Ikeda [:sotaro]]

(In reply to Mike Habicher [:mikeh] from comment #19)
> I see that these settings _are_ supported in JB[1] and KK[2], which could
> explain why zooming while recording works properly on (e.g.) the Nexus 4.
> 
> 1.
> http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/
> GonkNativeWindowClientJB.cpp#372
> 2.
> http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/
> GonkNativeWindowClientKK.cpp#359

These values are stored in GonkNativeWindow, but they are not used at all.

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to Mike Habicher [:mikeh] from comment #19)
> I see that these settings _are_ supported in JB[1] and KK[2], which could
> explain why zooming while recording works properly on (e.g.) the Nexus 4.

It says, nexus-4's camera hal works correctly as the camera hal says in camera-capability.

Comment 25

[Sotaro Ikeda [:sotaro]]

In b2g, preview zoom is controlled by camera app, not by GonkNativeWindow. Therefore, camera hal have to provide correct value for zooming.

Comment 26

[Justin D'Arcangelo [:justindarc]]

Sotaro: Setting the `zoom` attribute on `mozCamera` causes the preview stream to reflect the zoomed value up until a certain point at which it stops having any effect on the preview stream. In the Camera app, we use a CSS transform to scale the preview the rest of the way. Ideally, we should have an API that does one of two things:

a.) The preview stream always appears unzoomed (1.0x) and we handle the preview stream scaling 100% in the app code using a CSS transform

-or-

b.) The preview stream always correctly reflects the set zoom value so we don't have to apply any scaling in the app using CSS

However, currently, we have to use a hybrid of the two and have to detect at what point the `mozCamera` preview stream will no longer scale the image and it adds a tremendous amount of complexity to the Camera app.

Comment 27

[Justin D'Arcangelo [:justindarc]]

However, the issue in this particular bug has to do with the *recorded* video, not the preview stream. On Hamachi, *both* the recorded video and the preview stream are not scaling up to the reported 4.0x. This is the only device we've encountered so far that is exhibiting this behavior.

Comment 28

[Sotaro Ikeda [:sotaro]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #27)
> However, the issue in this particular bug has to do with the *recorded*
> video, not the preview stream. On Hamachi, *both* the recorded video and the
> preview stream are not scaling up to the reported 4.0x. This is the only
> device we've encountered so far that is exhibiting this behavior.

recorded video is totally not related to GonkNativeWindow(SurfaceTexture). Form it, the following 2 possibilities seems possible.
- zoom is not supported on hamachi.
- camera hal control is not correct.

Comment 29

[Sotaro Ikeda [:sotaro]]

If we want to add camera preview zoom control via GonkNativeWindow by camera hal, a strategy like Bug 976397 could be used. But it it not trivial change and intrinsically not fit well to b2g.

Comment 30

[Justin D'Arcangelo [:justindarc]]

Yeah, I didn't think it would be an easy fix. Is it possible to then have the preview stream not zoom at all then? Having the preview stream always appear un-zoomed would simplify the Camera app code greatly.

Comment 31

[Mike Habicher [:mikeh] (high bugzilla latency)]

Are we of the opinion then that we can't fix this, then?

Comment 32

[Justin D'Arcangelo [:justindarc]]

Mike: Is there any elegant way for you to put a conditional case in for Hamachi? Like a "zoom blacklist" or something? If the `zoomRatios` array only had a single 1.0 value in it on Hamachi, that would cause zoom to be disabled in the Camera app.

Comment 33

[Jason Smith [:jsmith]]

(In reply to Mike Habicher [:mikeh] from comment #31)
> Are we of the opinion then that we can't fix this, then?

I don't think that's an option. We can't place ourselves into a situation where blatantly exposing UX that won't work on certain devices. If certain devices can't support it, then we need a solution that allows us to disable the functionality.

Comment 34

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Justin D'Arcangelo [:justindarc] from comment #32)
>
> Mike: Is there any elegant way for you to put a conditional case in for
> Hamachi? Like a "zoom blacklist" or something? If the `zoomRatios` array
> only had a single 1.0 value in it on Hamachi, that would cause zoom to be
> disabled in the Camera app.

Justin, we can add a property override[1] that disables zoom. Probably something like 'ro.moz.cam.0.disable_zoom'. The only problem is that (I believe) this setting isn't applied to a device unless it's fully flashed, which is something we don't do for hamachi. On that device, we only flash gecko and gaia.

1. https://github.com/mozilla-b2g/android-device-hamachi/blob/master/full_hamachi.mk

Comment 35

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Part 1 - Link to PR to add ro. property to disable flaky zoom on hamachi

A subsequent patch will be required to actually pick up this property in Gecko.

Comment 36

[Sotaro Ikeda [:sotaro]]

About new b2g devices, it might be better to discuss about camera hal and zoom on partners.

Comment 37

[Chris Lee [:clee]]

(In reply to Hema Koka [:hema] from comment #17)
> Chris, 
> 
> Since Sri is out, we need product input here...
> 
> Is it acceptable for picture and video modes to have different zoom limits?
> 
> Thanks
> Hema

From the product perspective, we should ensure the zoom levels accurately match the video stream that is being captured.  If this means 1.8x vs. 4.0x, that's acceptable.  If it means we can't zoom because the device doesn't support it, then we should remove the ability to zoom in video capture mode.  

If the answer is, 'this is a hard bug to fix, but we believe there is a realistic solution', then I'd file a separate bug with the proposed solution and add it to our 'bug backlog' (not a feature) given the work here to finish the original implementation is still incomplete.

Comment 38

[Hema Koka [:hema]]

Just to be clear, if the hardware reports that zoom is not supported, we disable that feature...

This particular issue is with hamachi where it is reporting 1.0x - 4.0x zoom values but those don't work fine on video

For 1.4, lets look into disabling zoom for hamachi and file a separate bug for figuring out the right solution for devices that have the inconsistent/flaky zooming capabilities.

Thanks
Hema

Comment 39

[Michael Wu [:mwu]]

Comment on attachment 8404348 [details]
Part 1 - Link to PR to add ro. property to disable flaky zoom on hamachi

AFAICT, this is a POVB bug. The hamachi camera hal needs to be fixed in order to support zoom or to stop reporting support for zoom. If there's going to be a 1.4 update for hamachi, the vendor will need to fix it one way or another. Won't hurt to ask for a fixed camera hal, at any rate.

Comment 40

[Hema Koka [:hema]]

Wayne, 

Please help include the right vendor contact for this: https://bugzilla.mozilla.org/show_bug.cgi?id=991962#c39

Thanks
Hema

Comment 41

[Wayne Chang [:wchang]]

(In reply to Hema Koka [:hema] from comment #40)
> Wayne, 
> 
> Please help include the right vendor contact for this:
> https://bugzilla.mozilla.org/show_bug.cgi?id=991962#c39
> 
> Thanks
> Hema

Added and will speak to him in person.

Just another thought, couldn't we have zoom option disabled by default except for flame (which we should have full/more control over) and have OEMs configure the zoom option as they build for their target devices?

If we can confirm our implementation is correct then if things are specific to hardware, the OEMs can hack around making it work on those devices.

Comment 42

[Mike Habicher [:mikeh] (high bugzilla latency)]

Dseassigning myself as this is POVB.

Comment 43

[Jason Smith [:jsmith]]

Removing the blocking flag since this is now a vendor issue.

Comment 44

[Mike Habicher [:mikeh] (high bugzilla latency)]

Hi Wayne, this bug is getting a bit stale. Just wondering if there was any update here.

Comment 45

[Wayne Chang [:wchang]]

Have not heard back but since they weren't taking 1.4 it's likely that there had been no actions here.

Comment 46

[Mike Habicher [:mikeh] (high bugzilla latency)]

Thanks, Wayne. Making closed. If you believe this is incorrect, please reopen.