Closed Bug 823798 Opened 7 years ago Closed 7 years ago

Route FM radio through analog path

Categories

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

All
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
blocking-basecamp -
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- fixed

People

(Reporter: mwu, Assigned: philikon)

References

Details

Attachments

(2 files, 3 obsolete files)

Using the analog path is more efficient.
Unfortunately this patch doesn't allow the radio to turn back on.
This enables playback through the analog path.

It's a little weird though - if I enable fm radio on the audio policy manager with hw.fm.isAnalog = true, it doesn't work, but with hw.fm.isAnalog = false, the audio is routed correctly. That's why this code immediately turns hw.fm.isAnalog off after it's done.
Attachment #694653 - Attachment is obsolete: true
Attachment #694936 - Flags: review?(justin.lebar+bug)
Blocks: 823605
blocking-basecamp: --- → ?
Priority: -- → P1
Target Milestone: --- → B2G C3 (12dec-1jan)
Marking as a soft blocker. Efficiency in this case would be great but, as we understand the bug at this point, it won't block the release.
blocking-basecamp: ? → -
tracking-b2g18: --- → +
This is the public version of bug 823605 . As long as bug 823605 blocks, this needs to block too.
blocking-basecamp: - → ?
blocking-basecamp: ? → +
Michael, Justin is out until early January, can someone else help review this?
Comment on attachment 694936 [details] [diff] [review]
Play FM radio through analog path, v2

We need to resolve bug 823605 comment 11.
Attachment #694936 - Flags: review?(justin.lebar+bug)
(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> We need to resolve bug 823605 comment 11.

That has an answer now, looks like. Who can take help this bug along?
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
Philikon said he could take this.
Assignee: mwu → philipp
Attached patch v2.1 (obsolete) — Splinter Review
mwu's patch slightly changed according to bug 823605 comment 12.
Attachment #694936 - Attachment is obsolete: true
Attachment #698162 - Flags: review?(justin.lebar+bug)
(In reply to Philipp von Weitershausen [:philikon] from comment #9)
> Created attachment 698162 [details] [diff] [review]
> v2.1
> 
> mwu's patch slightly changed according to bug 823605 comment 12.

This patch doesn't look any different AFAICT
(In reply to Michael Wu [:mwu] from comment #10)
> > mwu's patch slightly changed according to bug 823605 comment 12.
> 
> This patch doesn't look any different AFAICT

It's missing the problematic

  property_set("hw.fm.isAnalog", "true");

line [1]. I tested the FM radio app and it works for me. Am I missing something?

[1] https://bugzilla.mozilla.org/attachment.cgi?oldid=694936&action=interdiff&newid=698162&headers=1
hw.fm.isAnalog needs to be true for this to work on non-otoro/unagi hardware. Otoro/unagi's audio hal doesn't seem to support the analog fm path properly. I think the right way/work around would be to check whether hw.fm.isAnalog is set to true or not, and configure the audio path accordingly.
(In reply to Michael Wu [:mwu] from comment #12)
> hw.fm.isAnalog needs to be true for this to work on non-otoro/unagi
> hardware. Otoro/unagi's audio hal doesn't seem to support the analog fm path
> properly. I think the right way/work around would be to check whether
> hw.fm.isAnalog is set to true or not, and configure the audio path
> accordingly.

So hw.fm.isAnalog is not something we set but a device-level configuration setting?
I feel like mwu is probably a better reviewer here; I'm totally out of the loop on this stuff.
Attachment #698162 - Flags: review?(justin.lebar+bug)
Comment on attachment 698162 [details] [diff] [review]
v2.1

>diff --git a/hal/gonk/GonkFMRadio.cpp b/hal/gonk/GonkFMRadio.cpp

>+  property_set("hw.fm.isAnalog", "false");

Please remove this line and double-check that it still works on unagi/otoro.
Attachment #698162 - Flags: review-
(In reply to Chris Jones [:cjones] [:warhammer] from comment #15)
> Comment on attachment 698162 [details] [diff] [review]
> v2.1
> 
> >diff --git a/hal/gonk/GonkFMRadio.cpp b/hal/gonk/GonkFMRadio.cpp
> 
> >+  property_set("hw.fm.isAnalog", "false");
> 
> Please remove this line and double-check that it still works on unagi/otoro.

Gah, I removed the wrong line in the patch didn't I. It does not work with *this* line removed. :/
(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> > >+  property_set("hw.fm.isAnalog", "false");
> > 
> > Please remove this line and double-check that it still works on unagi/otoro.
> 
> Gah, I removed the wrong line in the patch didn't I. It does not work with
> *this* line removed. :/

Something else is going, though. Bug 827590.
Michael Vines will sort this out, reassigning, and not blocking basecamp on this, but tef+.
Assignee: philipp → mvines
blocking-b2g: --- → tef+
blocking-basecamp: + → -
Attached patch v3Splinter Review
Simplest way fwd seems to be to introduce another device specific "quirk" property that we set at build time, and have Gecko choose digital vs analog path depending on that.
Assignee: mvines → philipp
Attachment #698162 - Attachment is obsolete: true
Attachment #702104 - Flags: review?(mwu)
Patches for android-device-otoro and android-device-unagi.
Attachment #702106 - Flags: review?(mwu)
Attachment #702106 - Flags: review?(mwu) → review+
Comment on attachment 702104 [details] [diff] [review]
v3

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

Not sure if I'm the best reviewer since this is derived from my old patch, but ok.

::: hal/gonk/GonkFMRadio.cpp
@@ +166,5 @@
> +  property_get("ro.moz.fm.noAnalog", propval, "");
> +  bool noAnalog = !strcmp(propval, "true");
> +
> +  if (noAnalog) {
> +    rc = setControl(V4L2_CID_PRIVATE_TAVARUA_SET_AUDIO_PATH, FM_DIGITAL_PATH);

This might be a bit cleaner with a ternary selecting between digital/analog.
Attachment #702104 - Flags: review?(mwu) → review+
Can we get this landed in the b2g18 tree today?
(In reply to Michael Vines [:m1] from comment #24)
> Can we get this landed in the b2g18 tree today?

It's been landed, see comment 22.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/00ab761c3545
oh cools, sorry
No longer blocks: 823605
Duplicate of this bug: 823605
https://hg.mozilla.org/mozilla-central/rev/9eecc4f1a318
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.
You need to log in before you can comment on or make changes to this bug.