Closed Bug 1007349 Opened 8 years ago Closed 8 years ago

Fix invalid timestamp in orientation angle calculation

Categories

(Core Graveyard :: Widget: Gonk, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.4+, firefox30 wontfix, firefox31 wontfix, firefox32 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
mozilla32
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: tkundu, Assigned: tkundu)

References

()

Details

(Whiteboard: [caf priority: p2][CR 624040])

Attachments

(1 file, 1 obsolete file)

Attached patch Fix inavalid timestamps (obsolete) — Splinter Review
Assignee: nobody → tkundu
Status: NEW → ASSIGNED
Attachment #8418980 - Flags: review?(mwu)
blocking-b2g: --- → 1.4?
Whiteboard: [CR 624040]
Component: Bluetooth → Widget: Gonk
Product: Firefox OS → Core
Comment on attachment 8418980 [details] [diff] [review]
Fix inavalid timestamps

Not so sure about the whitespace removal at the top, but everything else looks good. Thanks!
Attachment #8418980 - Flags: review?(mwu) → review+
Comment on attachment 8418980 [details] [diff] [review]
Fix inavalid timestamps

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

::: widget/gonk/ProcessOrientation.cpp
@@ +185,1 @@
>    const float timeDeltaMS = (now - then) * 0.000001f;

The event.timestamp() return microseconds.
http://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prtime.h#39

I think we get problem of this line "timeDeltaMS = (now - then) * 0.000001f".
We need to check all time calculation expression.
Attachment #8418980 - Flags: feedback?(tkundu)
Attachment #8418980 - Flags: feedback?(mwu)
blocking-b2g: 1.4? → 1.4+
(In reply to Jerry Shih[:jerry] from comment #3)
> Comment on attachment 8418980 [details] [diff] [review]
> Fix inavalid timestamps
> 
> Review of attachment 8418980 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/ProcessOrientation.cpp
> @@ +185,1 @@
> >    const float timeDeltaMS = (now - then) * 0.000001f;
> 
> The event.timestamp() return microseconds.
> http://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prtime.h#39
> 
> I think we get problem of this line "timeDeltaMS = (now - then) * 0.000001f".
> We need to check all time calculation expression.

Thanks for pointing this . Here is my analysis: 

1) I can see that PRTIME is nothing but 64bit integer which is supposed to store only microseconds.
2) But GonkSensor retrives time in nanosecond from hardware sensor callback and store it in 64bit PRTIME in [2] 
3) I think that Android is also doing same thing here in [3]


[1] http://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prtime.h#48
[2] http://dxr.mozilla.org/mozilla-central/source/hal/gonk/GonkSensor.cpp#122
[3] http://opengrok.qualcomm.com/source/xref/b2g_kk_3.5/frameworks/base/policy/src/com/android/internal/policy/impl/WindowOrientationListener.java#422


I think that we all good once I fix NIT from #comment 2 and upload a new patch . Please suggest.
Flags: needinfo?(hshih)
(In reply to Tapas Kumar Kundu from comment #4)
> (In reply to Jerry Shih[:jerry] from comment #3)

> [3]
> http://opengrok.qualcomm.com/source/xref/b2g_kk_3.5/frameworks/base/policy/
> src/com/android/internal/policy/impl/WindowOrientationListener.java#422
> 

This link should be :
https://www.codeaurora.org/cgit/quic/la/platform/frameworks/base/tree/policy/src/com/android/internal/policy/impl/WindowOrientationListener.java?h=b2g_kk_3.5#n422

Sorry for confusion
Attachment #8418980 - Flags: feedback?(tkundu) → feedback+
I don't know whether we should convert the ns timestamp to microsecond at:
http://dxr.mozilla.org/mozilla-central/source/hal/gonk/GonkSensor.cpp#122
If all GonkSensor receives ns timestamp, maybe we need to check all b2g code which use event.timestamp().
Flags: needinfo?(hshih) → needinfo?(mwu)
Attachment #8418980 - Flags: feedback?(mwu) → feedback-
(In reply to Jerry Shih[:jerry] from comment #6)
> I don't know whether we should convert the ns timestamp to microsecond at:
> http://dxr.mozilla.org/mozilla-central/source/hal/gonk/GonkSensor.cpp#122
> If all GonkSensor receives ns timestamp, maybe we need to check all b2g code
> which use event.timestamp().

You can see definition of sensors_event_t in

https://www.codeaurora.org/cgit/quic/la/platform/hardware/libhardware/tree/include/hardware/sensors.h?h=LNX.LF.3.5.2.2#n826

I think that we can upload orientation timer fix here. For other sensors, we can create a separate bug id (if needed) . Please suggest
Hi Michael,

Do we really use PRTime timestamp as microsecond or just a 64bit int value container?
http://dxr.mozilla.org/mozilla-central/source/hal/sandbox/PHal.ipdl#48
Is it worth to convert ns time to us time at [1]? Maybe we need to modify all code which use SensorData.timestamp.

[1]
http://dxr.mozilla.org/mozilla-central/source/hal/gonk/GonkSensor.cpp#122
Looks like we just use it as a 64bit int. PRTime might not be the appropriate type here.
Flags: needinfo?(mwu)
I suggest landing this patch and follow up on fixing the PRTime issue in a separate bug. Converting to a true PRTime is probably the right solution, but we probably need to audit the users to make sure they're all doing the right thing.
NIT fixed and carrying r+ from mwu from #comment 2.
Attachment #8418980 - Attachment is obsolete: true
Attachment #8420359 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ae78bb875714
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Not enough information with current STR to write test case to address bug.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(rmitchell)
Test case added in moztrap to ensure portrait to landscape transitions are smooth and quick:

https://moztrap.mozilla.org/manage/case/14344/
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(rmitchell)
Flags: in-moztrap+
Whiteboard: [CR 624040] → [caf priority: p2][CR 624040]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.