Closed Bug 1053173 Opened 6 years ago Closed 6 years ago

[FM Radio] Convert FM tune thread to LazyIdleThread

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(1 file, 4 obsolete files)

The thread is an implementation detail of Gonk. It shouldn't be located in DOM.
Attached patch [01] Move FM tune thread to HAL (obsolete) — Splinter Review
I'm actually interested in removing these threads completely and executing the code on the I/O thread if that's possible. The patch is a first step towards this goal.
Attachment #8472308 - Flags: review?(pzhang)
Comment on attachment 8472308 [details] [diff] [review]
[01] Move FM tune thread to HAL

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

r=me, please address these nits, thanks.

Hi, @mwu, would you mind to take a look the gonk part? thanks.

::: dom/fmradio/FMRadioService.cpp
@@ -230,4 @@
>      // Fix Bug 796733. DisableFMRadio should be called before
>      // SetFmRadioAudioEnabled to prevent the annoying beep sound.
>      DisableFMRadio();
>      fmRadioService->EnableAudio(false);

Please inline |fmRadioService|.

@@ -235,5 @@
>      return NS_OK;
>    }
>  };
>  
> -class SetFrequencyRunnable MOZ_FINAL : public nsRunnable

We removed the gonk call runnable, so please remove the friend class declaration[1], and update the comment[2] that contains |SetFrequencyRunnable|.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/FMRadioService.h#144
[2] http://mxr.mozilla.org/mozilla-central/source/dom/fmradio/FMRadioService.cpp#566

@@ -251,5 @@
> -private:
> -  int32_t mFrequency;
> -};
> -
> -class SeekRunnable MOZ_FINAL : public nsRunnable

Same for SeekRunnable.

::: hal/gonk/GonkFMRadio.cpp
@@ +399,5 @@
> +class SeekRunnable MOZ_FINAL : public nsRunnable
> +{
> +public:
> +  SeekRunnable(FMRadioSeekDirection aDirection)
> +  : mDirection(aDirection)

Nits, indention.

@@ +471,5 @@
> +class SetFrequencyRunnable MOZ_FINAL : public nsRunnable
> +{
> +public:
> +  SetFrequencyRunnable(int32_t aFrequency)
> +  : mFrequency(aFrequency)

Nits, indention.
Attachment #8472308 - Flags: review?(pzhang)
Attachment #8472308 - Flags: review?(mwu)
Attachment #8472308 - Flags: review+
Hi, Thomas, do you have a plan to move EnableRunnable and DisableRunnable from DOM to gonk?
I don't think FM tuning should be on the IO thread. It doesn't contend for the same resources, and for certain operations like seeking, the call can take several seconds simply waiting for the hardware.
(In reply to Pin Zhang [:pzhang] from comment #3)
> Hi, Thomas, do you have a plan to move EnableRunnable and DisableRunnable
> from DOM to gonk?

Not yet.

(In reply to Michael Wu [:mwu] from comment #4)
> I don't think FM tuning should be on the IO thread. It doesn't contend for
> the same resources, and for certain operations like seeking, the call can
> take several seconds simply waiting for the hardware.

My intention was to reduce the number of b2g's threads to save resources. I read in the V4L documentation that some of the ioctl ops support |select|. But I first wanted to do some experiments if this works in practice.
I've hacked on a few V4L2 drivers and I don't remember seeing anything that would allow select to work on the seeking/tuning ops we're using. Also not sure how select would actually work on ioctl ops. You can read/write to /dev/radio0 for RDS, and select does work for that.
Comment on attachment 8472308 [details] [diff] [review]
[01] Move FM tune thread to HAL

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

I don't think it's possible to move this to the IO thread, so moving this around doesn't make sense to me. r- unless you figure out a way to make that work.
Attachment #8472308 - Flags: review?(mwu) → review-
(In reply to Michael Wu [:mwu] from comment #7)
> Comment on attachment 8472308 [details] [diff] [review]
> [01] Move FM tune thread to HAL
> 
> Review of attachment 8472308 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think it's possible to move this to the IO thread, so moving this
> around doesn't make sense to me. r- unless you figure out a way to make that
> work.

OK, no problem. If you say it's not possible, then it probably isn't.

But did you ever consider using LazyIdleThreads for these internal threads? Since users usually don't change frequency very often, the thread could get removed after a few seconds, and re-created on demand. That might save a few KiB on low-end systems.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #8)
> But did you ever consider using LazyIdleThreads for these internal threads?
> Since users usually don't change frequency very often, the thread could get
> removed after a few seconds, and re-created on demand. That might save a few
> KiB on low-end systems.

Sounds like a good idea! Feel free to assign to me if you file a bug for that. (or morph this one)
Summary: [FM Radio] Move FM tune thread to HAL → [FM Radio] Convert FM tune thread to LazyIdleThread
Submitting this patch again, because I still think this is an implementation detail of the Gonk backend.

Otherwise, we can also convert the DOM tune thread to a LazyIdleThread.
Attachment #8472308 - Attachment is obsolete: true
Attachment #8484176 - Flags: review?(mwu)
I choose 5 seconds as tune timeout. This is probably enough time for a user to decide whether to stay with the station or seek to the next one.
Attachment #8484177 - Flags: review?(mwu)
Regarding the use of polling on the I/O thread.

From reading the docs [1][2] it looks like it's possible to poll for VIDIOC_DQBUF.

The ioctl for VIDIOC_DQBUF in our code reads buffers with type V4L2_BUF_TYPE_PRIVATE.

Is is possible to only poll for these buffer types? This would allow for moving that ioctl to the I/O thread. The MSM init code (initMsmFMRadio) could run on the new LazyIdleThread, and the extra radio thread could be removed entirely.

[1] http://linuxtv.org/downloads/v4l-dvb-apis/func-poll.html
[2] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-qbuf.html
Changes since v1:

  - add missing #include statement
Attachment #8484177 - Attachment is obsolete: true
Attachment #8484177 - Flags: review?(mwu)
Attachment #8484201 - Flags: review?(mwu)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #12)
> Regarding the use of polling on the I/O thread.
> 
> From reading the docs [1][2] it looks like it's possible to poll for
> VIDIOC_DQBUF.
> 
> The ioctl for VIDIOC_DQBUF in our code reads buffers with type
> V4L2_BUF_TYPE_PRIVATE.
> 
> Is is possible to only poll for these buffer types? This would allow for
> moving that ioctl to the I/O thread. The MSM init code (initMsmFMRadio)
> could run on the new LazyIdleThread, and the extra radio thread could be
> removed entirely.
> 
> [1] http://linuxtv.org/downloads/v4l-dvb-apis/func-poll.html
> [2] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-qbuf.html

Yeah I think in theory we can move the msm thread to the io thread. There's two concerns here -

1. If there's support on the driver side for poll.
2. If the event rate is low enough to make it worth it. The MSM FM drivers send all kinds of events. Hopefully the event rate is low and not unnecessarily waking up the CPU.
Hmm I think the msm fm drivers actually don't support poll. I have a patch which adds support for poll for the purpose of reading RDS data, but not VIDIOC_DQBUF events.
Comment on attachment 8484176 [details] [diff] [review]
[01] Bug 1053173: Move FM tune thread to HAL, r=pzhang

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

Sorry about the late response - had to think about the pro/cons of this.

I think moving the thread to the HAL side is just going to make it harder to understand what this code is doing. Not that it's very easy right now, but for simple non-MSM FM drivers, the FM HAL mostly does a number of ioctl calls and returns. I'd like to keep all the logic and state in one place so it's easy to follow.

The tuning thread looks like a detail of the Gonk implementation because the HAL interface is asynchronous. I'm not sure that's actually the API we want there. Even with MSM FM drivers, there are parts that are *very* synchronous. We had to move most of MSM FM init to a separate thread, which made the code harder to understand.
Attachment #8484176 - Flags: review?(mwu)
Depends on: 1085383
Attachment #8484201 - Flags: review?(mwu)
Attachment #8484176 - Attachment is obsolete: true
Attachment #8484201 - Attachment is obsolete: true
Attachment #8522918 - Flags: review?(mwu)
Tested on flame-kk.
Comment on attachment 8522918 [details] [diff] [review]
[01] Bug 1053173: Use |LazyIdleThread| for FM tuning

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

Always good to see comments that have perfect line breaking. :)
Attachment #8522918 - Flags: review?(mwu) → review+
https://hg.mozilla.org/mozilla-central/rev/3d4cf044cf6b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.