659188 - Camera support for Android

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

This bug deals with implementing the moz-device protocol for Android.

My patch queue is at http://hg.mozilla.org/users/fdesre_mozilla.com/camera-patches

Test page at http://people.mozilla.org/~fdesre/capture/

Current status:
- data flows from java to the c++ side and is asked by the stream listener
- nsRawReader decodes the header and packets without reporting errors
- but nothing is displayed, and the input stream is not closed when stopping the preview.

I wonder is the preview format on the android side (NV21, see http://developer.android.com/reference/android/graphics/ImageFormat.html) is the one our decoder expects, but the frame size matches, so we should display something, even if only noise. I tested with NV16 with similar results.

Comment 1

[Timothy B. Terriberry (:derf)]

NV21 is not the same as what we're expecting. See http://www.fourcc.org/yuv.php
In particular, it has the U and V planes interleaved, where as we expect each plane to be in one contiguous block.

Also, bug 634557 add conversion+scaling routines which expect the rows in each buffer to be aligned to a 16-byte boundary and padded with at least 16 pixels on a side (as these things are true for all of our other decoders). This may be something to watch out for.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

this fixes it https://hg.mozilla.org/users/blassey_mozilla.com/patches/file/71a69051897e/fix_camera_video

Comment 3

[[:fabrice] Fabrice Desré]

I updated my patch queue at http://hg.mozilla.org/users/fdesre_mozilla.com/camera-patches

We can now preview: http://people.mozilla.org/~fdesre/capture/camera-1.png
and
http://people.mozilla.org/~fdesre/capture/camera-2.png

Tested on my g2 running 2.3, the frame rate looks decent but we have about 1 second of delay.

Remaining issues:
- the input stream is never closed when we remove the <video/> element from the DOM. This means taht we don't close the camera on the Android side.

- I hardcoded the mInfo.mDisplay field in nsRawReader.cpp. This value overrides the width and height set on the video element, which it should not IMHO.

- We must rotate the image ourselves depending on the phone orientation.

Comment 4

[[:fabrice] Fabrice Desré]

I updated my patch queue and posted a build at http://people.mozilla.com/~fdesre/capture/fennec-camera.apk

This works quite well on the G2 (2.3) and the Vibrant (2.2).

I still need to figure out how to not hardcode mInfo.mDisplay field in nsRawReader.cpp

Another needed improvement is that we currently capture the video preview, which is low-res. We should rather do a full resolution capture; this could be a followup bug.

If anyone can test on other devices, that would be great!

Comment 5

[Alon Zakai (:azakai)]

I built with that patch queue and tested on an HTC Desire S (2.3.3). When I click 'capture', a window opens but the camera input is black - nothing shows up (the camera itself is fine since it works in the native camera app).

Second issue, in logcat I see many

I/GeckoEvent( 2581): SensorEvent type = 3 AK8973 Orientation sensor -77.0 -0.0 1.0

which continue to fire even after closing the camera input, and even after closing the web page and tab.

Comment 6

[Josh Matthews [:jdm]]

I didn't see black, but there wasn't any camera input in the box either, on two different Galaxy S phones. In the log I saw "E/Camera: error 0", but couldn't figure out where in the source that was coming from.

Comment 7

[Alon Zakai (:azakai)]

I tested the linked build from comment 4, and both of the problems I mentioned in the previous comment do not happen. Is that build not built with the latest patch queue perhaps?

That build seems pretty good, only issue is as fabrice told me on irc, there is lag in the camera output.

Comment 8

[Alon Zakai (:azakai)]

I see the same thing as jdm in comment 6 on a Galaxy S.

Comment 9

[Alon Zakai (:azakai)]

On the Galaxy S, the failure appears to be due to

W/GeckoAppJava( 2892): Camera error : java.lang.NumberFormatException: unable to parse ' 30' as integer

W/System.err( 2892): java.lang.NumberFormatException: unable to parse ' 30' as integer

W/System.err( 2892): 	at java.lang.Integer.parse(Integer.java:374)

W/System.err( 2892): 	at java.lang.Integer.parseInt(Integer.java:363)

W/System.err( 2892): 	at java.lang.Integer.parseInt(Integer.java:323)

W/System.err( 2892): 	at android.hardware.Camera$Parameters.splitInt(Camera.java:1557)

W/System.err( 2892): 	at android.hardware.Camera$Parameters.getSupportedPreviewFrameRates(Camera.java:1028)

W/System.err( 2892): 	at org.mozilla.gecko.GeckoAppShell.initCamera(GeckoAppShell.java:1348)

W/System.err( 2892): 	at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)

W/System.err( 2892): 	at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)

W/System.err( 2892): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:395)

W/System.err( 2892): 	at org.mozilla.gecko.GeckoApp$2.run(GeckoApp.java:170)

which happens when we call

  Iterator<Integer> it = params.getSupportedPreviewFrameRates().iterator();

Looks like a bug in the Android java code... and I don't quite see a way to work around it.

Comment 10

[[:fabrice] Fabrice Desré]

I updated the patch queue to current m-c and uploaded a new build as well.

I'm not seeing the java exception on my galaxy S (2.2), but I implemented a workaround that just try to set the preferred frame rate.

About the orientation log:
I/GeckoEvent( 2581): SensorEvent type = 3 AK8973 Orientation sensor -77.0 -0.0 1.0

The capture picker dialog uses orientation events to rotate the video. The event listener is removed when we close the dialog, so I'm not sure why we still see these lines after closing the dialog.

nsBuiltinDecoderStateMachine.cpp is probably where we need to tweak things to reduce the lag and more aggressively skip frames. With the posted build, the lag is acceptable on my galaxy S (around 1 to 2 seconds), but much worse on my g2 running 2.3

Comment 11

[[:fabrice] Fabrice Desré]

One more thing: we get the first frame displayed very quickly (almost no lag), they we freeze, and the "live" view starts with some lag.

Comment 12

[Alon Zakai (:azakai)]

Attachment: patch queue update

fabrice, here is a patch for your patch queue. Fixes some breakage here and there. The more interesting breakages were the new nsDOMFileFile and that Android now defines LINUX.

Comment 13

[Alon Zakai (:azakai)]

Attachment: patch queue update v2

(Forgot to hg qref for the last patch.)

Comment 14

[Josh Matthews [:jdm]]

Comment on attachment 550543 [details] [diff] [review]
patch queue update v2

>++cheez
>++
> +already_AddRefed<V4l2CaptureProvider> GetV4l2CaptureProvider() {

???

Comment 15

[Alon Zakai (:azakai)]

(In reply to comment #14)
> Comment on attachment 550543 [details] [diff] [review] [diff] [details] [review]
> patch queue update v2
> 
> >++cheez
> >++
> > +already_AddRefed<V4l2CaptureProvider> GetV4l2CaptureProvider() {
> 
> ???

That was a quick way to check if that code is being built, that is, to confirm what #defines are active. Please ignore ;)

Might be some other stuff in that patch, I didn't polish it.

Meanwhile my build finished. I still get the black camera input on my htc desire s, just like in comment 5.

Comment 16

[Alon Zakai (:azakai)]

On my galaxy S, I get the same error as before as in comment 9. I thought there was a workaround for that?

Comment 17

[[:fabrice] Fabrice Desré]

(In reply to comment #14)
> Comment on attachment 550543 [details] [diff] [review] [diff] [details] [review]
> patch queue update v2
> 
> >++cheez
> >++
> > +already_AddRefed<V4l2CaptureProvider> GetV4l2CaptureProvider() {
> 
> ???

This is not built for Android. Just the beginning of a generic v4l2 implementation

Comment 18

[Alon Zakai (:azakai)]

Attachment: patch queue update v3

I didn't realize the patch queue was recently updated. Turns out most of the previous update patch is not needed. Attached is the LINUX definition bit that is.

With this, I now crash on my galaxy S when I try to open camera input, without a clear reason,

I/Gecko   ( 3170): GetAndroidCaptureProvider() instance=0x4b91c8f8
I/Gecko   ( 3170): AndroidCaptureProvider::AndroidCaptureProvider()
W/GeckoAppJava( 3170): initCamera(video/x-raw-yuv, 320x240) on thread 11
D/CameraService( 2208): CameraService::connect E (pid 3170, client 0x412e0)
D/CameraService( 2208): Client::Client E (pid 3170)
E/SecCamera( 2208): Inside initCamera and forebly connecting to tvout service...
I/TvOut-ITvOutService( 2224): tvout onTransact 2
I/TvOut-ITvOutService( 2224): tvout onTransact 3
I/TvOut-ITvOutService( 2224): tvout onTransact 4
I/TvOut-ITvOutService( 2224): tvout onTransact 5
I/TvOut-ITvOutService( 2224): tvout onTransact 6
E/SecCamera( 2208): AVATAR_CHANGE inside initCamera and Connected to TvOut Service
E/SecCamera( 2208): initCamera: m_cam_fd(15), m_jpeg_fd(0)
I/SecCamera( 2208): Name of input channel[0] is CE147
E/SecCamera( 2208): initCamera: m_cam_fd2(22)
I/SecCamera( 2208): Name of input channel[0] is CE147
D/CameraHardwareSec( 2208): frame rate:30, mPreviewFrameRateMicrosec:33333
E/SecCamera( 2208): SetRotate(angle(0))
E/SecCamera( 2208): setRecordingSize(width(640), height(480))
I/ShotCommon( 2208): Picture width(2560), height(1920)
I/ShotSingle( 2208): ShotSingle created: pid=2208
V/MediaPlayer( 2208): constructor
V/MediaPlayer( 2208): setDataSource(/system/media/audio/ui/camera_click.ogg)
V/MediaPlayerService( 2208): Client(8) constructor
V/MediaPlayerService( 2208): Create new client(8) from pid 2208, url=/system/media/audio/ui/camera_click.ogg, connId=8
V/MediaPlayerService( 2208): setDataSource(/system/media/audio/ui/camera_click.ogg)
D/MediaPlayerService( 2208): getPlayerType : url = /system/media/audio/ui/camera_click.ogg, filePath = /system/media/audio/ui/camera_click.ogg
V/MediaPlayerService( 2208): player type = 3
V/MediaPlayerService( 2208):  create VorbisPlayer
V/MediaPlayerService( 2208):  setDataSource
V/MediaPlayer( 2208): MediaPlayer::setAudioStreamType
V/MediaPlayer( 2208): prepare
V/MediaPlayerService( 2208): [8] setAudioStreamType(7)
V/MediaPlayerService( 2208): [8] prepareAsync
V/MediaPlayerService( 2208): [8] notify (0x41868, 1, 0, 0)
E/MediaPlayer( 2208): message received msg=1, ext1=0, ext2=0
V/MediaPlayer( 2208): prepared
V/MediaPlayer( 2208): signal application thread
V/MediaPlayer( 2208): prepare complete - status=0
V/MediaPlayer( 2208): constructor
V/MediaPlayer( 2208): setDataSource(/system/media/audio/ui/VideoRecord.ogg)
V/MediaPlayerService( 2208): Client(9) constructor
V/MediaPlayerService( 2208): Create new client(9) from pid 2208, url=/system/media/audio/ui/VideoRecord.ogg, connId=9
V/MediaPlayerService( 2208): setDataSource(/system/media/audio/ui/VideoRecord.ogg)
D/MediaPlayerService( 2208): getPlayerType : url = /system/media/audio/ui/VideoRecord.ogg, filePath = /system/media/audio/ui/VideoRecord.ogg
V/MediaPlayerService( 2208): player type = 3
V/MediaPlayerService( 2208):  create VorbisPlayer
V/MediaPlayerService( 2208):  setDataSource
V/MediaPlayer( 2208): MediaPlayer::setAudioStreamType
V/MediaPlayer( 2208): prepare
V/MediaPlayerService( 2208): [9] setAudioStreamType(7)
V/MediaPlayerService( 2208): [9] prepareAsync
V/MediaPlayerService( 2208): [9] notify (0x3d510, 1, 0, 0)
E/MediaPlayer( 2208): message received msg=1, ext1=0, ext2=0
V/MediaPlayer( 2208): prepared
V/MediaPlayer( 2208): signal application thread
V/MediaPlayer( 2208): prepare complete - status=0
D/CameraService( 2208): Client::Client X (pid 3170)
D/CameraService( 2208): CameraService::connect X
D/CameraService( 2208): getParameters(AppShutterSound=0;anti-shake=0;antibanding=auto;antibanding-values=auto, 50hz, 60hz, off;beauty-shot=0;blur=0;brightness=4;camera-id=1;chk_dataline=0;contrast=2;digi-zoom=0;effect=none;effect-values=none, mono, negative, sepia;flash-mode=off;flash-mode-values=off;focus-mode=auto;focus-mode-values=auto,macro;iso=auto;jpeg-quality=100;jpeg-thumbnail-height=192;jpeg-thumbnail-quality=100;jpeg-thumbnail-size-values=160x120;jpeg-thumbnail-width=256;metering=center;picture-format=jpeg;picture-format-values=jpeg;picture-size=2560x1920;picture-size-values=2560x1920,2048x1536,1600x1200,640x480,2560x1536,2048x1232,1600x960,800x480;preview-format=yuv420sp;preview-format-values=yuv420sp;preview-frame-rate=30;preview-frame-rate-values=15, 30;preview-size=640x480;preview-size-values=640x480,800x480;rotation=0;saturation=2;scene-mode=auto;scene-mode-values=auto, portrait, landscape, night, beach, snow, sunset, fireworks, sports, party, candlelight;sharpness=2;slow_ae=off;smart-auto=0;video_recording_gamma=off
W/GeckoAppJava( 3170): Available preview fps:
W/GeckoAppJava( 3170): Available preview image size:
W/GeckoAppJava( 3170): size: 640x480
W/GeckoAppJava( 3170):   ok 
W/GeckoAppJava( 3170): size: 800x480
D/CameraService( 2208): setParameters(anti-shake=0;picture-size-values=2560x1920,2048x1536,1600x1200,640x480,2560x1536,2048x1232,1600x960,800x480;AppShutterSound=0;antibanding=auto;blur=0;digi-zoom=0;metering=center;sharpness=2;contrast=2;brightness=4;whitebalance=auto;jpeg-thumbnail-height=192;scene-mode=auto;jpeg-quality=100;preview-format-values=yuv420sp;rotation=0;jpeg-thumbnail-quality=100;focus-mode=auto;beauty-shot=0;preview-format=yuv420sp;vintagemode=off;preview-size=640x480;iso=auto;slow_ae=off;picture-format-values=jpeg;camera-id=1;flash-mode-values=off;preview-frame-rate-values=15, 30;chk_dataline=0;preview-frame-rate=10;flash-mode=off;effect-values=none, mono, negative, sepia;focus-mode-values=auto,macro;picture-size=2560x1920;effect=none;saturation=2;jpeg-thumbnail-width=256;whitebalance-values=auto, incandescent, fluorescent, daylight, cloudy-daylight;scene-mode-values=auto, portrait, landscape, night, beach, snow, sunset, fireworks, sports, party, candlelight;picture-format=jpeg;vtmode=0;jpeg-thumbnail-size-values=1
D/CameraHardwareSec( 2208): frame rate:10, mPreviewFrameRateMicrosec:100000
E/SecCamera( 2208): SetRotate(angle(0))
E/SecCamera( 2208): setRecordingSize(width(640), height(480))
I/DEBUG   ( 3165): debuggerd committing suicide to free the zombie!
I/DEBUG   ( 3207): debuggerd: Sep 28 2010 21:15:30
D/Zygote  ( 2207): Process 3170 exited cleanly (11)
D/CameraService( 2208): Client::~Client E (pid 2208, client 0x412e0)
V/MediaPlayer( 2208): disconnect
V/MediaPlayerService( 2208): disconnect(9) from pid 2208
I/ActivityManager( 2263): Process org.mozilla.fennec_unofficial (pid 3170) has died.
I/WindowManager( 2263): WIN DEATH: Window{47cba670 org.mozilla.fennec_unofficial/org.mozilla.fennec_unofficial.App paused=false}
I/WindowManager( 2263): WIN DEATH: Window{47cdce90 SurfaceView paused=false}
I/UsageStats( 2263): Unexpected resume of com.sec.android.app.twlauncher while already resumed in org.mozilla.fennec_unofficial

Comment 19

[Alon Zakai (:azakai)]

On my htc desire s, I still get a black screen. The relevant part of the output appears to be

W/GeckoAppJava(12582): Camera: 480x320 @ 15fps. format is 17

W/GeckoAppJava(12582): Camera preview started

I/Gecko   (12582): AndroidCaptureProvider::~AndroidCaptureProvider()

W/GeckoAppJava(12582): Camera preview: 230400 bytes on thread 1

D/SensorGUI( 1355): SensorChannel(): mSendFd = 251, mReceiveFd = 206

D/SensorGUI(12582): SensorChannel(const Parcel& data): mSendFd = -1, mReceiveFd = 108

D/Sensors ( 1355): Enable akm: en = 1

D/Sensors ( 1355): mEnabled = 0x1

D/Sensors ( 1355): Enable akm: en = 1

D/Sensors ( 1355): mEnabled = 0x1

D/Sensors ( 1355): After modify mEnabled = 0x5

I/Gecko   (12582): AndroidCameraInputStream::CloseWithStatus() status=0x804b0002

W/GeckoAppJava(12582): closeCamera() on thread 12

D/QualcommCameraHardware( 1252): stopPreview: E

D/QualcommCameraHardware( 1252): stopPreviewInternal E: 1

D/QualcommCameraHardware( 1252): cancelAutoFocusInternal E

W/mm-camera( 1252): VFE_RESET_ACK illegal ctrl->state 25

Comment 20

[Alon Zakai (:azakai)]

fabrice informs me that you need

  ac_add_options --enable-raw

in your mozconfig for this to work. Perhaps these patches should switch to enabling raw by default on mobile?

With that, I got it to work on my galaxy s. Lag is maybe 0.5 seconds, which is not too bad. The lag is very bad on an htc desire, though. But since it works basically ok on the galaxy S, I am guessing it isn't a problem in our code.

There are frequent hangs - GC perhaps?

In portrait mode on the galaxy S the display was not positioned correctly and the buttons didn't work. In landscape it works though. On the HTC desire, positioning is ok in portrait but the buttons don't work like on the galaxy S.

The image that appears after clicking OK is flipped in both X and Y for some reason, but the preview is ok. This happens on both phones.

Comment 21

[Alon Zakai (:azakai)]

(In reply to Alon Zakai (:azakai) from comment #20)
> on my galaxy s. Lag is maybe 0.5 seconds, which
> is not too bad. The lag is very bad on an htc desire, though. But since it
> works basically ok on the galaxy S, I am guessing it isn't a problem in our
> code.
> 

I may have been wrong about that. On the galaxy s (tiny lag) I see that the video queue in nsBuiltinDecoderStateMachine stays at a reasonable size of 1. But on the htc desire (horrible lag) the video queue increases in size, for example at frame 750 the video queue is of size 235. This is my first time looking at this code so maybe I am way off, but this looks bad.

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

are you looking at the size of mVideoQueue in the state machine?

Comment 23

[Alon Zakai (:azakai)]

(In reply to Brad Lassey [:blassey][[e-mail]] from comment #22)
> are you looking at the size of mVideoQueue in the state machine?

Yes.

Comment 24

[Alon Zakai (:azakai)]

Problem #1 with the lag is due to that queue growing in size. Not sure if it's the right solution, but making ScheduleStateMachine use a small amount of ms in the mRealTime case fixes that (for a realtime stream, we don't want to wait before reading another frame from the queue) and some other small fixes.

However, while this makes the frames get read from the video queue, they are not rendered. I can't actually figure out how rendering works here even in the normal case. nsMediaDecoder does mImageContainer->SetCurrentImage() on the new Image data. SetCurrentImage calls CurrentImageChanged, but neither of those functions directly do any rendering, just bookkeeping. I can't find any calls to invalidate or anything like that.

Comment 25

[Alon Zakai (:azakai)]

Chris, can you perhaps explain a bit about how rendering works in the media code (comment 24)?

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Brad Lassey [:blassey][[e-mail]] from comment #22)
> are you looking at the size of mVideoQueue in the state machine?

There should never be more than 10 frames in mVideoQueue.

The decode/rendering pipeline works like this:

1. nsBuiltinDecoderStateMachine::DecodeLoop() runs in a loop reading frames from the reader, by calling mReader->DecodeVideoFrame(), provided the mVideoQueue has fewer than AMPLE_VIDEO_FRAMES (10) in it [1]. If there are more than 10 frames in mVideoQueue, we don't call mReader->DecodeVideoFrame().

2. On the state machine thread (which is shared amongst all playing media), we periodically call nsBuiltinDecoderStateMachine::AdvanceFrame() [2], which figures out what the current time in the media is, and pops video frames which are late off mVideoQueue. It then displays the current frame if it's changed by calling RenderFrame() [3]. RenderFrame() calls down to nsMediaDecoder which sets the current image on the video element's ImageContainer. We then update the media's current time (by calling UpdatePlaybackPosition(), and if the current time has changed, we'll dispatch an event to calll nsBuiltinDecoder::PlaybackPositionChanged on the main thread, and that calls Invalidate().

3. We then look at the timestamp of the next frame in the queue (if we have one) and schedule the state machine thread to run at that time, or in 40ms if we don't have one.

So if you're seeing more than 10 frames queued in mVideoQueue, there's something wrong. Maybe nsRawReader::DecodeVideoFrame() is dumping a whole lot of frames into the video queue rather than just 1?

[1] http://mxr.mozilla.org/mozilla-central/source/content/media/nsBuiltinDecoderStateMachine.cpp#396
[2] http://mxr.mozilla.org/mozilla-central/source/content/media/nsBuiltinDecoderStateMachine.cpp#1608
[3] http://mxr.mozilla.org/mozilla-central/source/content/media/nsBuiltinDecoderStateMachine.cpp#1570

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

Can you attach your latest patches here so I can take a look at them?

Comment 28

[[:fabrice] Fabrice Desré]

Chris,

nsRawReader::DecodeVideoFrame() onlys pushes one frame at a time [1].

I updated the patch queue at http://hg.mozilla.org/users/fdesre_mozilla.com/camera-patches 
The android-camera.patch file contains the relevant changes.

[1] http://mxr.mozilla.org/mozilla-central/source/content/media/raw/nsRawReader.cpp#251

Comment 29

[Alon Zakai (:azakai)]

Not sure if this is helpful, but when I was tinkering with the code, I think that the important things to prevent queuing too many frames were

-    while (clock_time >= frame->mTime) {
+    while (mRealTime || clock_time >= frame->mTime) {

(that is, keep pulling frames as long as there are any, when in realtime mode - would be better to just get the last one though, I guess) in nsBuiltinDecoderStateMachine::AdvanceFrame, and also

+  if (mRealTime)
+    ms = 300;

in nsBuiltinDecoderStateMachine::ScheduleStateMachine (which prevents scheduling the timer for several seconds in the future - not sure why this was needed).

Comment 30

[[:fabrice] Fabrice Desré]

Attachment: Patch

Doug, please review the Android and input stream bits.
Chris, please review the state machine changes.

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 559647 [details] [diff] [review]
Patch

Review of attachment 559647 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I've been meaning to look into this myself, but I've been swamped dealing with bug 545812 and dependencies.

The changes you're proposing to nsBuiltinDecoderStateMachine basically force it to display 3.3fps in the raw case, but still decode video as fast as possible (I assume at 24fps?). You should lower the fps being produced by the camera, rather than wasting CPU time decoding frames just to throw them away. Or is the rendering pipeline the bottleneck here?

There should be no difference between the logic required to play a non-camera video in a timely fashion, and playing video from the camera. The logic to skip frames should work in either case.

I think Matthew Gregan is going to tweak the logic frame skipping logic in bug 643454 and/or bug 669556 which may help here.

Comment 32

[[:fabrice] Fabrice Desré]

> The changes you're proposing to nsBuiltinDecoderStateMachine basically force
> it to display 3.3fps in the raw case, but still decode video as fast as
> possible (I assume at 24fps?). You should lower the fps being produced by
> the camera, rather than wasting CPU time decoding frames just to throw them
> away. Or is the rendering pipeline the bottleneck here?

Instead of forcing |ms = 300| in camera mode, I just set it to |ms = 1| and get good results. It looks that we only lag when we take the |ms == 0| branch.
 
> There should be no difference between the logic required to play a
> non-camera video in a timely fashion, and playing video from the camera. The
> logic to skip frames should work in either case.

I agree!

Comment 33

[[:fabrice] Fabrice Desré]

Chris,

So when asking the camera 25fps, we get ms=40 almost all the time except that from time to time you get around 20000ms, hence the lag we see. I'm not sure why we get such a huge delay, since the input frames are still coming in. I made a change to not exceed 40ms :

http://hg.mozilla.org/users/fdesre_mozilla.com/camera-patches/file/e93f9467000b/android-camera.patch#l161

Comment 34

[Chris Pearce [:cpearce (Not reading bugmail)]]

Ok, so the only time way we can get non-zero ms in nsBuiltinDecoderStateMachine::ScheduleStateMachine(aUsecs) is when we pass non-zero aUsecs into ScheduleStateMachine(). The only place we do that is at http://mxr.mozilla.org/mozilla-central/source/content/media/nsBuiltinDecoderStateMachine.cpp#1764. So either the remainingTime calculation in AdvanceFrame() is wrong, or inappropriate for realtime for some reason, or the timestamps which it uses to calculate the frame duration is wrong.

We calculate aUsecs as ((start time of next frame) - (time now)), and in the case where we don't have a next frame, we assume 40ms (hence why you're seeing ms=40 most of the time). Understanding why we're seeing ms=20000 here may help understanding the underlying problem.

Comment 35

[Doug Turner (:dougt)]

Comment on attachment 559647 [details] [diff] [review]
Patch

blassey is back.  he touched alot of this last.

Comment 36

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 559647 [details] [diff] [review]
Patch

Review of attachment 559647 [details] [diff] [review]:
-----------------------------------------------------------------

::: embedding/android/GeckoAppShell.java
@@ +1396,5 @@
> + 
> +    static int[] initCamera(String aContentType, int aWidth, int aHeight) {
> +        Log.w("GeckoAppJava", "initCamera(" + aContentType + ", " + aWidth + "x" + aHeight + ") on thread " + Thread.currentThread().getId());
> +
> +        // XXX needs API level 9

if we need API level 9, check for it here

@@ +1398,5 @@
> +        Log.w("GeckoAppJava", "initCamera(" + aContentType + ", " + aWidth + "x" + aHeight + ") on thread " + Thread.currentThread().getId());
> +
> +        // XXX needs API level 9
> +        //if (android.hardware.Camera.getNumberOfCameras() == 0)
> +        //  return false;

why is this commented out?

::: netwerk/protocol/device/AndroidCaptureProvider.cpp
@@ +66,5 @@
> +  PRUint24 codecID;
> +};
> +
> +// This is Arc's draft from wiki.xiph.org/OggYUV
> +struct nsRawVideoHeader {

this should probably be in a header file to be shared

@@ +124,5 @@
> +  mContentType = aContentType;
> +  mWidth = aParams->width;
> +  mHeight = aParams->height;
> +
> +  if (XRE_GetProcessType() != GeckoProcessType_Content) {

I assume you want to check that this is the chrome process, in which case this should be:
if (XRE_GetProcessType() == GeckoProcessType_Default)

otherwise, the test will succeed in the jetpack process for example

@@ +136,5 @@
> +    }
> +    
> +  }
> +  else {
> +    return NS_ERROR_NOT_IMPLEMENTED;

you should return early at the top rather than do this, so:
if (XRE_GetProcessType() != GeckoProcessType_Default)
  return NS_ERROR_NOT_IMPLEMENTED;

and of course that would get rid of the above if as well

@@ +170,5 @@
> +derf: However, we're expecting to have a W*H block of data for Y, then a (W/2)*(H/2) block of data for Cb, then a (W/2)*(H/2) block of data for Cr.
> +derf: (people don't follow consistent conventions on which of U and V maps to Cb and Cr... I'd try V=Cb and U=Cr first, but swap them if that looks wrong)
> +  
> +  memcpy() the Y plane, and de-interlace the CrCb
> +  */

get rid of this comment, or at least boil it down

@@ +204,5 @@
> +
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP AndroidCameraInputStream::IsNonBlocking(PRBool *aBlock) {

this should probably be aNonBlock

@@ +231,5 @@
> +    header.headerPacketID = 0;
> +    header.codecID = 0x595556; // "YUV"
> +    header.majorVersion = 0;
> +    header.minorVersion = 1;
> +    header.options = 1 | 1 << 2;

comment what those options are

@@ +292,5 @@
> +
> +/**
> + * must be called on the main (java) thread
> + */
> +void AndroidCameraInputStream::doClose() {

you can add an assertion for this

::: netwerk/protocol/device/CameraStreamImpl.cpp
@@ +90,5 @@
> +            if (mCamera1)
> +                res = mCamera1;
> +            else
> +                res = mCamera1 = new CameraStreamImpl(aCamera);
> +            break;

can there only be 2 cameras?

@@ +110,5 @@
> +{
> +    NS_WARNING("CameraStreamImpl::~CameraStreamImpl()");
> +}
> +
> +bool CameraStreamImpl::Init(const nsCString& contentType, const PRUint32& width, const PRUint32& height, FrameCallback* aCallback)

I'd like to keep all JNI code in the AndroidBridge, please move this there

@@ +144,5 @@
> +
> +    return res;
> +}
> +
> +void CameraStreamImpl::Close() {

move this too

Comment 37

[[:fabrice] Fabrice Desré]

Attachment: addressing comments

> can there only be 2 cameras?

For now we can settle with having only one back and one front camera I think. We don't have yet the UI to choose which one to use.

Addressed all the other comments from Brad.

Comment 38

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 562191 [details] [diff] [review]
addressing comments

Review of attachment 562191 [details] [diff] [review]:
-----------------------------------------------------------------

r=blassey with nits addressed

::: content/media/raw/nsRawReader.cpp
@@ +43,4 @@
>  #include "nsRawDecoder.h"
>  #include "VideoUtils.h"
>  
> +static const PRUint32 RAW_ID = 0x595556;

move this to nsRawStructs.h

::: embedding/android/GeckoAppShell.java
@@ +1592,5 @@
> + 
> +    static int[] initCamera(String aContentType, int aWidth, int aHeight) {
> +        Log.w("GeckoAppJava", "initCamera(" + aContentType + ", " + aWidth + "x" + aHeight + ") on thread " + Thread.currentThread().getId());
> +
> +        // XXX needs API level 9

test for API level 9 and return early if not

@@ +1594,5 @@
> +        Log.w("GeckoAppJava", "initCamera(" + aContentType + ", " + aWidth + "x" + aHeight + ") on thread " + Thread.currentThread().getId());
> +
> +        // XXX needs API level 9
> +        //if (android.hardware.Camera.getNumberOfCameras() == 0)
> +        //  return false;

remove this commented out code

@@ +1603,5 @@
> +        // [3] = fps
> +        int[] result = new int[4];
> +        result[0] = 0;
> +
> +        if (Build.VERSION.SDK_INT >= 9) {

if you return early for <9 above you won't need to test here

@@ +1615,5 @@
> +            params.setPreviewFormat(ImageFormat.NV21);
> +
> +            // use the preview fps closest to 25 fps.
> +            int fpsDelta = 1000;
> +            Log.w("GeckoAppJava", "Available preview fps:");

this sounds like an info log rather than a warning, use the right level (Log.i())

@@ +1623,5 @@
> +                    int nFps = it.next();
> +                    if (Math.abs(nFps - kPreferedFps) < fpsDelta) {
> +                        fpsDelta = Math.abs(nFps - kPreferedFps);
> +                        params.setPreviewFrameRate(nFps);
> +                        Log.w("GeckoAppJava", "Setting Preview fps: " + nFps);

Log.i

@@ +1626,5 @@
> +                        params.setPreviewFrameRate(nFps);
> +                        Log.w("GeckoAppJava", "Setting Preview fps: " + nFps);
> +                    }
> +                }
> +            } catch(Exception e) {

what throws here? let's catch explicit exceptions

@@ +1631,5 @@
> +                params.setPreviewFrameRate(kPreferedFps);
> +            }
> +
> +            // set up the closest preview size available
> +            Log.w("GeckoAppJava", "Available preview image size:");

Log.i

@@ +1637,5 @@
> +            int sizeDelta = 10000000;
> +            int bufferSize = 0;
> +            while (sit.hasNext()) {
> +                android.hardware.Camera.Size size = sit.next();
> +                Log.w("GeckoAppJava", "size: " + size.width + "x" + size.height);

Log.i

@@ +1640,5 @@
> +                android.hardware.Camera.Size size = sit.next();
> +                Log.w("GeckoAppJava", "size: " + size.width + "x" + size.height);
> +                if (Math.abs(size.width * size.height - aWidth * aHeight) < sizeDelta) {
> +                    sizeDelta = Math.abs(size.width * size.height - aWidth * aHeight);
> +                    Log.w("GeckoAppJava", "  ok ");

this log seems uninformative, drop it. In general there seems to be too much logging in this code, please reconsider it all

@@ +1647,5 @@
> +                }
> +            }
> +            
> +            sCamera.setParameters(params);
> +            sCameraBuffer = new byte[(bufferSize * 12) / 8];

comment on your math here

@@ +1658,5 @@
> +            });
> +            sCamera.startPreview();
> +            params = sCamera.getParameters();
> +            Log.w("GeckoAppJava", "Camera: " + params.getPreviewSize().width + "x" + params.getPreviewSize().height +
> +                  " @ " + params.getPreviewFrameRate() + "fps. format is " + params.getPreviewFormat());

Log.i

@@ +1664,5 @@
> +            result[1] = params.getPreviewSize().width;
> +            result[2] = params.getPreviewSize().height;
> +            result[3] = params.getPreviewFrameRate();
> +
> +            Log.w("GeckoAppJava", "Camera preview started");

Log.i

@@ +1665,5 @@
> +            result[2] = params.getPreviewSize().height;
> +            result[3] = params.getPreviewFrameRate();
> +
> +            Log.w("GeckoAppJava", "Camera preview started");
> +        } catch(Exception e) {

what throws? Let's catch specific exceptions

@@ +1666,5 @@
> +            result[3] = params.getPreviewFrameRate();
> +
> +            Log.w("GeckoAppJava", "Camera preview started");
> +        } catch(Exception e) {
> +            Log.w("GeckoAppJava", "Camera error : " + e);

Log.e

@@ +1667,5 @@
> +
> +            Log.w("GeckoAppJava", "Camera preview started");
> +        } catch(Exception e) {
> +            Log.w("GeckoAppJava", "Camera error : " + e);
> +            e.printStackTrace();

Log.e(GeckoAppJava", "Camera error : ", e); will print the stack trace, use it

@@ +1684,5 @@
> +                sCamera = null;
> +                sCameraBuffer = null;
> +            }
> +        } catch(Exception e) {
> +            e.printStackTrace();

what throws here? Catch specific exceptions

Also, use Log.e("GeckoAppJava", "closeCamera exception", e); instead of e.printStackTrace

::: widget/src/android/AndroidBridge.cpp
@@ +1189,5 @@
> +}
> +
> +void
> +AndroidBridge::CloseCamera() {
> +    // XXX TODO : add the camera # to JNI

why not do this now?

Comment 39

[[:fabrice] Fabrice Desré]

Attachment: updated patch

Addressed Brad's last comments, and added the ability to choose which camera to use. Currently we just set it to 0 (default camera) in nsDeviceChannel.

Comment 40

[[:fabrice] Fabrice Desré]

Attachment: updated patch

I forgot to qref before posting patch...

Comment 41

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 562491 [details] [diff] [review]
updated patch

Review of attachment 562491 [details] [diff] [review]:
-----------------------------------------------------------------

r=cpearce with the changes below.

Add a pref to disable camera support, preffed off by default. Also add a pref to toggle "realtime" mode, so that we can easily compare them. That will make it convenient to debug the issue with camera->decoder integration, while preventing future bitrot.

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +228,4 @@
>      NS_ABORT_IF_FALSE(NS_SUCCEEDED(res), "Can't create media state machine thread");
>    }
>    gStateMachineCount++;
> +  mBufferingWait = mRealTime ? 0 : BUFFERING_WAIT;

Check the pref to enable "realtime" mode here, and set mRealTime to PR_FALSE if the pref is false. This pref should be equate to false on desktop (even if raw support is enabled).

::: content/media/nsBuiltinDecoderStateMachine.h
@@ +624,5 @@
>    // by the decoder monitor.
>    PRPackedBool mDecodeThreadWaiting;
>  
> +  // true is we are decoding a realtime stream, like a camera stream
> +  bool mRealTime;

PRBool. At least until mwu works his magic.

Comment 42

[[:fabrice] Fabrice Desré]

Attachment: patch with prefs

Updated per Chris comments.

We now have two prefs: device.camera and realtime.decoder that are only true in fennec builds.

Comment 43

[Mark Finkle (:mfinkle) (use needinfo?)]

In other prefs, I see Mozilla use the xxx.enabled = true | false pattern. Could we use that for "device.camera.enabled" ?

Comment 44

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 562582 [details] [diff] [review]
patch with prefs

Review of attachment 562582 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/app/mobile.js
@@ +680,5 @@
>  pref("browser.dom.window.dump.enabled", false);
> +
> +// controls if we want camera support
> +pref("device.camera", true);
> +pref("realtime.decoder", true);

Along the same lines as mfinkle's comment, I'd change realtime.decoder to media.realtime_decoder.enabled, so it matches the other media.* prefs.

Comment 45

[[:fabrice] Fabrice Desré]

pushed with updated preferences:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0df8c6fcea9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ecc8009b371
https://hg.mozilla.org/integration/mozilla-inbound/rev/78b3f8faf4e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc1c5649927

Comment 46

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0df8c6fcea9e
https://hg.mozilla.org/mozilla-central/rev/8ecc8009b371
https://hg.mozilla.org/mozilla-central/rev/78b3f8faf4e3
https://hg.mozilla.org/mozilla-central/rev/5fc1c5649927

Comment 47

[Ed Morley [:emorley]]

Actually, both the comment 45 and comment 46 changesets are referring to bug 593891.

The correct changeset for this bug is:
https://hg.mozilla.org/mozilla-central/rev/5fc1c5649927

Comment 48

[Aaron Train [:aaronmt]]

Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111003 Firefox/10.0a1 Fennec/10.0a1
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a2) Gecko/20111003 Firefox/9.0a2 Fennec/9.0a2

Comment 49

[Matt Brubeck (:mbrubeck)]

Nominating for tracking-firefox9.  This is a new feature in update 9 with a number of known bugs (some already fixed for 9.0 and others not yet fixed).  We should decide whether to ship this or back it out before the next aurora->beta merge.

Comment 50

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Matt Brubeck (:mbrubeck) from comment #49)
> Nominating for tracking-firefox9.  This is a new feature in update 9 with a
> number of known bugs (some already fixed for 9.0 and others not yet fixed). 
> We should decide whether to ship this or back it out before the next
> aurora->beta merge.

Just so we have it documented somewhere. Does removing this feature (if needed) require a backout or a preference flip?

Comment 51

[Chris Pearce [:cpearce (Not reading bugmail)]]

This was supposed to land disabled by default, and can be enabled by setting a pref. No action should be required to ensure it's still disabled on any branch this is present on.

Comment 52

[Ian Melven :imelven]

spoke to Fabrice about this recently, no security concerns currently, setting sec-review-complete

Comment 53

[John Hammink]

Removing the 'in-litmus' flag as anyway now, we're in Moztrap.

Comment 54

[Gian-Carlo Pascutto [:gcp]]

Is this something we still use? Can we remove this?

Comment 55

[Brad Lassey [:blassey] (use needinfo?)]

I don't know. Maybe add a telemetry probe to find out?

Comment 56

[Gian-Carlo Pascutto [:gcp]]

Uh, no. Let's ask Fabrice if we still use this/this is still wanted after we added getUserMedia & WebRTC.

Comment 57

[[:fabrice] Fabrice Desré]

No, I don't think we want to keep that.

Comment 58

[Chris Pearce [:cpearce (Not reading bugmail)]]

Just to clarify, you're proposing we remove dom/media/RawReader? I've been wondering for a while if we still needed it...

Comment 59

[Gian-Carlo Pascutto [:gcp]]

RawReader only seems to be a small part of the patch here. I'm mostly interested in the Camera handling parts for Android. Being unmaintained, it's unlikely they work, which begged the question why the code is there at all.