[b2g-bluetooth] Failed to set audio stream volume to correct value by bluetooth headset

RESOLVED FIXED in Firefox 18

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gyeh, Assigned: gyeh)

Tracking

unspecified
mozilla19
ARM
Gonk (Firefox OS)
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

1.76 KB, patch
Details | Diff | Splinter Review
4.45 KB, patch
Details | Diff | Splinter Review
1.12 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Repro:
- Connect with a bluetooth headset
- Press volume up/down

Expected Behaviour:
- Audio stream volume should be set to assigned value from bluetooth headset

Actual Behaviour:
- Wrong volume value
(Assignee)

Updated

5 years ago
Assignee: nobody → gyeh
(Assignee)

Comment 1

5 years ago
Created attachment 674124 [details] [diff] [review]
Patch 1(v1): BluetoothHfpManager receive Vgs from headset
Attachment #674124 - Flags: review?(kyle)
(Assignee)

Comment 2

5 years ago
Created attachment 674125 [details] [diff] [review]
Patch 2(v1): Send Chrome event of type 'volumeset' when bluetooth headset adjust volume
Attachment #674125 - Flags: review?(21)
(Assignee)

Comment 3

5 years ago
Created attachment 674126 [details] [diff] [review]
Patch 3(v1): Add event handler for 'volumeset' in sound_manager.js
Attachment #674126 - Flags: review?(21)
Comment on attachment 674125 [details] [diff] [review]
Patch 2(v1): Send Chrome event of type 'volumeset' when bluetooth headset adjust volume

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

I'm not against the change but this code will break $GAIA/apps/system/js/hardware_buttons.js so r-. You may want to ask the r? to :djf next time too, he is the hardware button guy :)
Attachment #674125 - Flags: review?(21) → review-
Comment on attachment 674126 [details] [diff] [review]
Patch 3(v1): Add event handler for 'volumeset' in sound_manager.js

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

I guess this code will change depending on the mozChromeEvent you need to make hardware_buttons.js happy.
Attachment #674126 - Flags: review?(21)
Comment on attachment 674124 [details] [diff] [review]
Patch 1(v1): BluetoothHfpManager receive Vgs from headset

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

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +143,5 @@
>    StaticRefPtr<BluetoothHfpManagerObserver> sHfpObserver;
>    bool gInShutdown = false;
>    static nsCOMPtr<nsIThread> sHfpCommandThread;
>    static bool sStopSendingRingFlag = true;
> +  static bool sReceiveVgsFlag = false;

Do we really need to keep making these static? This isn't accessed like sStopSendingRingFlag, which is becoming a mamber anyways as of bug 804687. Seems like this could just be a member too?

@@ +225,5 @@
>  {
> +  float volume;
> +  nsCOMPtr<nsIAudioManager> am = do_GetService("@mozilla.org/telephony/audiomanager;1");
> +  if (!am) {
> +    NS_WARNING("Failed to get AudioManager Service!");

Don't fail in the constructor. Do this in a failable init function or something.

@@ +489,3 @@
>        newVgs *= 10;
> +      newVgs += tmp - '0';
> +    }

Why not just throw this into a ns*String and use ToInteger?
Attachment #674124 - Flags: review?(kyle) → review-
(Assignee)

Comment 8

5 years ago
Comment on attachment 674125 [details] [diff] [review]
Patch 2(v1): Send Chrome event of type 'volumeset' when bluetooth headset adjust volume

Please help to check. Thanks!
Attachment #674125 - Flags: review- → review?(dflanagan)
(Assignee)

Comment 9

5 years ago
Comment on attachment 674126 [details] [diff] [review]
Patch 3(v1): Add event handler for 'volumeset' in sound_manager.js

Please help to check. Thanks!
Attachment #674126 - Flags: review?(dflanagan)
(Assignee)

Comment 10

5 years ago
Created attachment 674506 [details] [diff] [review]
Patch 1(v2): BluetoothHfpManager receive Vgs from headset

- No return in constructor
- Make static variable into member variable (mReceiveVgsFlag)
- Use nsACString.ToInteger()
Attachment #674124 - Attachment is obsolete: true
Attachment #674506 - Flags: review?(kyle)
Comment on attachment 674506 [details] [diff] [review]
Patch 1(v2): BluetoothHfpManager receive Vgs from headset

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

You made the nsCOMPtr a member, but I don't see a header file? Does this patch have everything?

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +226,5 @@
> +  float volume;
> +  nsCOMPtr<nsIAudioManager> am = do_GetService("@mozilla.org/telephony/audiomanager;1");
> +  if (!am) {
> +    NS_WARNING("Failed to get AudioManager Service!");
> +    return;

This looks like it's still erroring in the constructor, instead of having an init function now?

@@ +482,5 @@
>      // HS volume range: [0, 15]
> +    nsCString vgsStr;
> +    while (length--) {
> +      vgsStr.Append(msg[index++]);
> +    }

Nit: Can't we just substr the string based on the length to pull out the number, and append that?
Attachment #674506 - Flags: review?(kyle)
(Assignee)

Comment 12

5 years ago
Created attachment 674539 [details] [diff] [review]
Patch 1(v3): BluetoothHfpManager receive Vgs from headset

Oops, my fault! All changes should be included in this patch now. Also move the initialization of mCurrentVgs to Init().
Attachment #674506 - Attachment is obsolete: true
Attachment #674539 - Flags: review?(kyle)
Comment on attachment 674125 [details] [diff] [review]
Patch 2(v1): Send Chrome event of type 'volumeset' when bluetooth headset adjust volume

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

I don't think that this will break hardware_buttons.js as Vivien has suggested.  You won't get any of the auto-repeat functionality from that module, of course, though I suppose that the headset itself might give you that.

I think the ideal thing would be to send a volumeset chrome event when the bluetooth headset first connects with the phone, and use that event simply to set the audio.volume.master setting.  Then, when the user alters the volume on their headset, it would be great if you could send the volume-up-press volume-up-release events as the current code does.  But since I don't know anything about how the gecko bluetooth code works, I don't know if that is possible.

It looks like this will work, though, so I'm going to give it an r+.  Please note, though, that I don't have a bluetooth headset and have not tested this.
Attachment #674125 - Flags: review?(dflanagan) → review+
Comment on attachment 674126 [details] [diff] [review]
Patch 3(v1): Add event handler for 'volumeset' in sound_manager.js

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

I would have preferred to have this code that handles the volumeset event in the same patch as the code that generates the event, so we would be certain that they got merged together. But that is not a big deal.

While reviewing this patch, I noticed a bug in sound_manager.js. On line 21, the default value for audio.volume.master is 5, when it should be 0.5.  Would you fix that as part of this patch so we don't have to file, fix, and review a new bug for it?

r+ with the bug fix and a comment about the volumeset event.

::: apps/system/js/sound_manager.js
@@ +9,5 @@
>    });
>    window.addEventListener('volumedown', function() {
>      changeVolume(-1);
>    });
> +  window.addEventListener('mozChromeEvent', function(e) {

Please add a comment before this line explaining that this chrome event is generated in shell.js in response to the bluetooth headset.

@@ +33,5 @@
>  
>    var activeTimeout = 0;
>    function changeVolume(delta) {
>      if (currentVolume == 0 ||
> +        ((currentVolume + delta) <= 0)) {

I think this is correct, but without a bluetooth headset I can't actually test it.

This part of sound_manager.js is really complex and ugly.  If I was writing it, I'd just allow volume to be an integer between -1 and 10 instead of 0 and 10.  -1 would me no sound and no vibration.  0 would mean vibration but no sound, and > 0 would be a volume. You don't have to fix that as part of this patch. I just want to note that it is confusing code, and this change makes it even more confusing.
Attachment #674126 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 15

5 years ago
(In reply to David Flanagan [:djf] from comment #13)
> I think the ideal thing would be to send a volumeset chrome event when the
> bluetooth headset first connects with the phone, and use that event simply
> to set the audio.volume.master setting.  Then, when the user alters the
> volume on their headset, it would be great if you could send the
> volume-up-press volume-up-release events as the current code does.  But
> since I don't know anything about how the gecko bluetooth code works, I
> don't know if that is possible.

Something different between volume button on bluetooth headset and volume button on the phone. When user press the volume button on bluetooth headset, we will get a new volume rather than +1 or -1. For example, the audio volume is set to 5 currently, when pressing volume-up button on bluetooth headset, it's volume will be set to 8 and send a message to notify the phone. If we press volume-up button again, it's volume will be set to 10 and send a message again. (The volume stream value is not fixed and depends on the implementation of bluetooth headsets. Two headsets may have different values when pressing volume buttons.) In this way, we have to (re)set audio volume whenever user press volume button from a bluetooth headset rather.

On the other hand, some bluetooth headset will send its volume when it first connects with the phone, and we notify observers of "bluetooth-volume-change", and will dispatch a mozChromeEvent typed 'volumeset' from shell.js to sound_manager.js. In sound_maanger.js, we'll show volume bar and update audio.volume.master setting.
(Assignee)

Comment 16

5 years ago
Created attachment 674943 [details] [diff] [review]
Patch 3 (Final Version): Add event handler for 'volumeset' in sound_manager.js, r=djf

Add comment and fix bug (initial value of currentVolume)
Attachment #674126 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Created attachment 674944 [details] [diff] [review]
Patch 1 (Final Version): BluetoothHfpManager receive Vgs from headset, r=qdot
Attachment #674539 - Attachment is obsolete: true
(Assignee)

Comment 18

5 years ago
Created attachment 674945 [details] [diff] [review]
Patch 2 (Final Version): Send Chrome event of type 'volumeset' when bluetooth headset adjust volume, r=djf
Attachment #674125 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
Created attachment 674970 [details] [diff] [review]
Patch 1 (Final Version): BluetoothHfpManager receive Vgs from headset, r=qdot

mercurial format patch
Attachment #674944 - Attachment is obsolete: true
(Assignee)

Comment 22

5 years ago
Created attachment 674971 [details] [diff] [review]
Patch 2 (Final Version): Send Chrome event of type 'volumeset' when bluetooth headset adjust volume, r=djf

mercurial format patch
Attachment #674945 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/814a4bca12eb
https://hg.mozilla.org/mozilla-central/rev/04c6d4d7e578
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.