Closed Bug 953034 Opened 6 years ago Closed 6 years ago

[Bluetooth][Certification][PTS][Bluedroid][1.3] AVRCP TC_TG_NFY_BV_04_C Failed

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 C2/1.4 S2(17jan)
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: echang, Assigned: shawnjohnjr)

References

Details

Attachments

(4 files, 5 obsolete files)

4.94 MB, application/x-zip-compressed
Details
6.40 KB, patch
Details | Diff | Splinter Review
3.00 KB, patch
Details | Diff | Splinter Review
6.43 KB, patch
Details | Diff | Splinter Review
Attached file TC_TG_NFY_BV_04_C.zip
### ENV
Nexus 4/Bluedroid
[2] https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/nightly/mozilla-central-mako/2013/12/2013-12-09-05-34-02/


### STR
1. PTS 5.0
2. TC_TC_NFY_BV_04_C

### Expected
Pass 

### Actual
Failed
Flags: needinfo?(jaliu)
Summary: [Bluetooth][Certification][Bluedroid] AVRCP TC_TG_NFY_BV_04_C Failed → [Bluetooth][Certification][PTS][Bluedroid] AVRCP TC_TG_NFY_BV_04_C Failed
Summary: [Bluetooth][Certification][PTS][Bluedroid] AVRCP TC_TG_NFY_BV_04_C Failed → [Bluetooth][Certification][PTS][Bluedroid][1.3] AVRCP TC_TG_NFY_BV_04_C Failed
Blocks: 951055
Assignee: nobody → shuang
This test case is to verify the Track Changed INTERIM response when no track is selected.
If no track was been selected, 	Track Changed event shall be replied with track id (0xffffffffffffffff)

Event ID: EVENT TRACK CHANGED
Track ID: 0xffffffffffffffff
If a track is selected, then return 0x0 in the response.
If no track is currently selected, then return 0xFFFFFFFFFFFFFFFF in the INTERIM response.
I think I made a mistake to implement UpdateRegisterNotification incorrectly. Based on Comment 3, it is supposed to follow spec to reply 0x0 or 0xFFFFFFFFFFFFFFFF. I have made a patch. This patch shall fix both of bug 953034 and 953035.
Flags: needinfo?(jaliu)
blocking-b2g: --- → 1.3?
triage: 1.3+ for cert blocker
blocking-b2g: 1.3? → 1.3+
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
In AVRCP 1.3 and 1.4, the identifier parameter of EVENT_TRACK_CHANGED is different
AVRCP 1.4:
If no track is selected, we shall return 0xFFFFFFFFFFFFFFFF, otherwise return 0x0 in the INTERRIM response.
The expanded text in version 1.4 is to allow for new UID feature.
As for AVRCP 1.3, we shall return 0xFFFFFFFF.
Since PTS enforces to check this part to comply with the most updated spec.
Attachment #8359718 - Flags: review?(echou)
Attachment #8359718 - Flags: feedback?(gyeh)
Comment on attachment 8360228 [details] [diff] [review]
Bug 953034 - [bluedroid] Reply RegisterNotification event EVENT_TRACK_CHANGED with correct parameter whether a track is selected or not

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

Please see my following comments. Thanks.

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +925,5 @@
>        param.play_status = (btrc_play_status_t)mPlayStatus;
>        break;
>      case BTRC_EVT_TRACK_CHANGE:
> +      // In AVRCP 1.3 and 1.4, the identifier parameter of EVENT_TRACK_CHANGED
> +      // is different

nit: Please add a period at the end of the sentence.

@@ +927,5 @@
>      case BTRC_EVT_TRACK_CHANGE:
> +      // In AVRCP 1.3 and 1.4, the identifier parameter of EVENT_TRACK_CHANGED
> +      // is different
> +      // AVRCP 1.4:
> +      // If no track is selected, we shall return 0xFFFFFFFFFFFFFFFF, otherwise

nit: Please merge the above two lines.

@@ +938,5 @@
>        // as uint8[8]. 56 = 8 * (BTRC_UID_SIZE -1)
> +      for (int index = 0; index < BTRC_UID_SIZE; ++index) {
> +        // We cannot easily check if a track is selected, so whenever A2DP is
> +        // streaming, we assume a track is selected
> +        if (mSinkState == BluetoothA2dpManager::SinkState::SINK_PLAYING) {

I suggest to check ControlPlayStatus rather than SinkState.

@@ +941,5 @@
> +        // streaming, we assume a track is selected
> +        if (mSinkState == BluetoothA2dpManager::SinkState::SINK_PLAYING) {
> +            param.track[index] = 0x0;
> +        } else {
> +            param.track[index] = 0xFF;

Should it be 0xFFFFFFFFFFFFFFFF?

@@ +947,4 @@
>        }
>        break;
>      case BTRC_EVT_PLAY_POS_CHANGED:
> +      // If no track is selected, return 0xFFFFFFFF in the INTERIM response

I see the comment but no implementation :(

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.h
@@ +45,5 @@
>    void UpdateMetaData(const nsAString& aTitle,
>                        const nsAString& aArtist,
>                        const nsAString& aAlbum,
> +                      uint64_t aMediaNumber,
> +                      uint64_t aTotalMediaCount,

Please also fix this in dom/bluetooth/bluez/BluetoothA2dpManager.h
> @@ +941,5 @@
> > +        // streaming, we assume a track is selected
> > +        if (mSinkState == BluetoothA2dpManager::SinkState::SINK_PLAYING) {
> > +            param.track[index] = 0x0;
> > +        } else {
> > +            param.track[index] = 0xFF;
> 
> Should it be 0xFFFFFFFFFFFFFFFF?
> 
It is 0xFFFFFFFFFFFFFFFF, 8 bytes.

> @@ +947,4 @@
> >        }
> >        break;
> >      case BTRC_EVT_PLAY_POS_CHANGED:
> > +      // If no track is selected, return 0xFFFFFFFF in the INTERIM response
> 
> I see the comment but no implementation :(
> 
> ::: dom/bluetooth/bluedroid/BluetoothA2dpManager.h
> @@ +45,5 @@
> >    void UpdateMetaData(const nsAString& aTitle,
> >                        const nsAString& aArtist,
> >                        const nsAString& aAlbum,
> > +                      uint64_t aMediaNumber,
> > +                      uint64_t aTotalMediaCount,
> 
> Please also fix this in dom/bluetooth/bluez/BluetoothA2dpManager.h
> @@ +947,4 @@
> >        }
> >        break;
> >      case BTRC_EVT_PLAY_POS_CHANGED:
> > +      // If no track is selected, return 0xFFFFFFFF in the INTERIM response
> 
> I see the comment but no implementation :(
>

I missed it since PTS did not test this part :( 
> ::: dom/bluetooth/bluedroid/BluetoothA2dpManager.h
> @@ +45,5 @@
> >    void UpdateMetaData(const nsAString& aTitle,
> >                        const nsAString& aArtist,
> >                        const nsAString& aAlbum,
> > +                      uint64_t aMediaNumber,
> > +                      uint64_t aTotalMediaCount,
> 
> Please also fix this in dom/bluetooth/bluez/BluetoothA2dpManager.h

Yes. I will do it.
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #12)
> > @@ +941,5 @@
> > > +        // streaming, we assume a track is selected
> > > +        if (mSinkState == BluetoothA2dpManager::SinkState::SINK_PLAYING) {
> > > +            param.track[index] = 0x0;
> > > +        } else {
> > > +            param.track[index] = 0xFF;
> > 
> > Should it be 0xFFFFFFFFFFFFFFFF?
> > 
> It is 0xFFFFFFFFFFFFFFFF, 8 bytes.

I think you're right. Just ignore this part. :)
Attachment #8360228 - Attachment is obsolete: true
Attachment #8360228 - Flags: review?(gyeh)
Attachment #8360852 - Attachment description: Bug 953034 - [bluedroid] Reply RegisterNotification event EVENT_TRACK_CHANGED with correct parameter whether a track is selected or not → Bug 953034 - Part1:[bluedroid] Reply RegisterNotification event EVENT_TRACK_CHANGED with correct parameter whether a track is selected or not
Attachment #8360909 - Attachment description: Bug 953034 - [bluez] Change Media number type to uint64_t → Bug 953034 - Part2: [bluez] Change Media number type to uint64_t
Comment on attachment 8360909 [details] [diff] [review]
Bug 953034 - Part2: [bluez] Change Media number type to uint64_t

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

Thanks.
Attachment #8360909 - Flags: review?(gyeh) → review+
Comment on attachment 8360852 [details] [diff] [review]
Bug 953034 - Part1:[bluedroid] Reply RegisterNotification event EVENT_TRACK_CHANGED with correct parameter whether a track is selected or not

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

r+ with nit addressed.

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +936,5 @@
>        // needs to convert to network big endian format since track stores
>        // as uint8[8]. 56 = 8 * (BTRC_UID_SIZE -1)
> +      for (int index = 0; index < BTRC_UID_SIZE; ++index) {
> +        // We cannot easily check if a track is selected, so whenever A2DP is
> +        // streaming, we assume a track is selected

nit: Please add a period at the end.
Attachment #8360852 - Flags: review?(gyeh) → review+
Attachment #8360947 - Attachment description: Bug 953034 - [bluedroid] Reply RegisterNotification event EVENT_TRACK_CHANGED with correct parameter whether a track is selected or not, r=echou, a=1.3+ → Bug 953034 - Part 1: Reply RegisterNotification event EVENT_TRACK_CHANGED with correct parameter whether a track is selected or not, r=echou, a=1.3+
Attachment #8360947 - Attachment description: Bug 953034 - Part 1: Reply RegisterNotification event EVENT_TRACK_CHANGED with correct parameter whether a track is selected or not, r=echou, a=1.3+ → (central)Bug 953034 - Part 1: Reply RegisterNotification event EVENT_TRACK_CHANGED with correct parameter whether a track is selected or not, r=echou, a=1.3+
Okay with this build.
│ Gaia      1318d1612299ba5d86820bbb6a65ae090f3b2fd6                         │
│ Gecko     http://hg.mozilla.org/releases/mozilla-aurora/rev/c40099a42c1f   │
│ BuildID   20140126004002                                                   │
│ Version   28.0a2                                                           │
│ ro.build.version.incremental=eng.archermind.20131114.105818                │
│ ro.build.date=Thu Nov 14 10:58:33 CST 2013
You need to log in before you can comment on or make changes to this bug.