Closed Bug 755546 Opened 12 years ago Closed 12 years ago

orientation change and video app don't work together

Categories

(Core :: Hardware Abstraction Layer (HAL), defect)

Product:
Core
Component:
Hardware Abstraction Layer (HAL)
Platform:
x86
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: djf, Assigned: cyu)

Details

Attachments

(2 files)

Fix incorrect deletion of sensor observer list in unregistration of one observer
1.22 KB, patch
cjones
: review+
Details | Diff | Splinter Review
Test code to verify the fix (manual, not for checking in).
1.47 KB, patch
Details | Diff | Splinter Review 
During the san diego workweek, we got orientation working on the phone: rotating the phone caused the screen to rotate too, except for apps that called mozLockOrientation().

Our desired behavior is that most gaia apps are locked in portrait-primary orientation but that Browser, Camera, Gallery and Video are unlocked and can rotate to match the way the user is holding the phone.

This was working at some point in the middle of the workweek. By Thursday night it was working for all apps except for the Gaia video app.  For some reason, that app would no longer rotate.

The video app has two unique features:

1) It is the only one with a <video> element
2) It is the only one that calls mozRequestFullScreen()

I've tried taking each of these things out of the app to see if it starts rotating again, but have had no luck. In part, though, that may be because this bug is being masked by bug #755489.
See bug #755532. Maybe I've got a bad build on my phone.
Actually, I take that last comment back. I see this bug on my sgs2 as well as the nexus s, and it is only my nexus s that has a bad build.
Bug 741088 patch part 2 introduced the checks for orientation locking. Locking will succeed if

1. the code has chrome privilege, or
2. some element requested full screen.

Video app can lock orientation because the video frame requests full screen. That's what I saw on the current code base.

:timdream said letting <html> element request full screen will work, though it's strange that the system app already owns the whole screen.
Cervantes: the mozLockOrientation code in nsScreen.cpp now has check to see if it was called from within an app.  The problem is that we're calling it from the system, which apparently does not count as an app.  Apps that lock themselves work.  Apps that rely on the homescreen and their manifest to lock themselves don't get locked because the homescreen doesn't pass the "is part of app" test.  I've emailed Vivien and Mounir about this, but haven't filed a bug yet because I'm not sure whether we should consider it a gecko bug or a gaia bug.

So what remains for this bug is that the video app does have locked orientation, even though it does not request it. Also, after running the video app, all other apps have their orientation locked.  It seems as if playing a video somehow interacts with the orientation sensor code and turns the sensors off.  I have a test app that allows me to lock and unlock orientation.  If I use that app to lock and unlock, that unfreezes things and the screen starts rotating again.
It seems using XMLHttpRequest in video app disables accelerometer. Here is the gdb excerpt:

#0  mozilla::hal::DisableSensorNotifications (aSensor=mozilla::hal::SENSOR_ACCELERATION) at /home/cervantes/hg/mozilla-central/hal/Hal.cpp:429
#1  0x413e6668 in mozilla::hal::UnregisterSensorObserver (aSensor=mozilla::hal::SENSOR_ACCELERATION, aObserver=0xc17624) at /home/cervantes/hg/mozilla-central/hal/Hal.cpp:465
#2  0x40c7a004 in nsDeviceSensors::RemoveWindowListener (this=0xc17620, aType=1, aWindow=0xc3eb68) at /home/cervantes/hg/mozilla-central/dom/system/nsDeviceSensors.cpp:174
#3  0x40bf6fea in nsGlobalWindow::SuspendTimeouts (this=0xc3eb68, aIncrease=1, aFreezeChildren=false) at /home/cervantes/hg/mozilla-central/dom/base/nsGlobalWindow.cpp:9738
#4  0x409fb3ac in nsXMLHttpRequest::Send (this=0x2bdb30, aCx=0xaf8098, aVariant=0x0, aBody=...) at /home/cervantes/hg/mozilla-central/content/base/src/nsXMLHttpRequest.cpp:3146
#5  0x413f0b36 in nsXMLHttpRequest::Send (this=0x2bdb30, aCx=0xaf8098, aBody=...) at /home/cervantes/hg/mozilla-central/content/base/src/nsXMLHttpRequest.h:382
#6  0x413f0ba0 in nsXMLHttpRequest::Send (this=0x2bdb30, aCx=0xaf8098, aRv=...) at /home/cervantes/hg/mozilla-central/content/base/src/nsXMLHttpRequest.h:392
#7  0x413f13bc in send (cx=0xaf8098, argc=0, vp=0x431680d8) at /home/cervantes/git/mozilla-b2g/B2G/objdir-gecko/dom/bindings/XMLHttpRequestBinding.cpp:179
#8  0x419cd734 in CallJSNative (cx=0xaf8098, args=..., construct=js::NO_CONSTRUCT) at /home/cervantes/hg/mozilla-central/js/src/jscntxtinlines.h:430
#9  js::InvokeKernel (cx=0xaf8098, args=..., construct=js::NO_CONSTRUCT) at /home/cervantes/hg/mozilla-central/js/src/jsinterp.cpp:523
#10 0x419e0824 in js::Interpret (cx=0xaf8098, entryFrame=0x43168028, interpMode=js::JSINTERP_NORMAL) at /home/cervantes/hg/mozilla-central/js/src/jsinterp.cpp:2744
#11 0x419cd4fc in js::RunScript (cx=0xaf8098, script=0x43a8eb88, fp=0x43168028) at /home/cervantes/hg/mozilla-central/js/src/jsinterp.cpp:479
... (omitted for brevity)

Linear accelerometer and gyroscope are also disabled in the loop in nsGlobalWindow::SuspendTimeouts(). Orientation then does not work because accelerometer is disabled. It is not clear why this method needs to do it. These sensor observers are not registered before. I think we should need to check for the case of unregistering a sensor observer that is not registered.
OK, it's not that the sensor observers are not registered. The observer has been registered before. The problem results from the following events

1. orientation sensor is enabled (maybe in the driver accelerometer is also enabled)
2. nsGlobalWindow enables accelerometer, linear accelerometer and gyroscope
3. When playing the video, nsGlobalWindow disables accelerometer, linear accelerometer and gyroscope (as in comment #5). Orientation is off now because it depends on accelerometer.

It looks like we cannot treat these sensors as really independent ones.
After taking a closer look at the problem, it turns out that there is a bug in hal code. Once any sensor is disabled, all sensor notifications are disabled. I will fix this bug in hal.
Assignee: nobody → cyu
Attachment #625595 - Flags: review?(jones.chris.g) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/a63dcee25c97
Assignee: cyu → nobody
Component: Widget → Hardware Abstraction Layer (HAL)
Keywords: checkin-needed
QA Contact: general → hal
Target Milestone: --- → mozilla15
Assignee: nobody → cyu
https://hg.mozilla.org/mozilla-central/rev/a63dcee25c97
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: