[MADAI] AudioOffloadPlayer: Glitch observed during mp3 clip startup

RESOLVED FIXED in Firefox 34, Firefox OS v2.0

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: vasanth, Assigned: vasanth)

Tracking

unspecified
mozilla34
x86_64
Linux
Points:
---
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox32 wontfix, firefox33 wontfix, firefox34 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8472321 [details] [diff] [review]
AudioOffloadPlayer-avoid-startup-glitch.patch

[Blocking Requested - why for this release]: Customer issue

Minor glitch observed from 0 to 0.2 seconds during playback of mp3 clip in offlaod mode.

Analysis
--------
1. We figure out whether platform supports offloading in MediaOMXReader [1]
2. Based on that info, MediaOmxDecoder pauses the state machine and starts AudioOffloadPlayer [2]
3. In between [1] and [2], if MediaDecoderStateMachine::StartPlayback() is started, then minor glitch is observed while switching from non offlaod to offload mode.

Suggested Fix
-------------
Attached patch (v1.4) makes MediaDecoderStateMachine::StartPlayback() a noop if  audio offloading is possible to avoid the glitch. 

Will upload V2.0 fix once the V1.4 fix is accepted.

[1] http://dxr.mozilla.org/mozilla-central/source/content/media/omx/MediaOmxReader.cpp#457
[2] http://dxr.mozilla.org/mozilla-central/source/content/media/omx/MediaOmxDecoder.cpp#79
Attachment #8472321 - Flags: review?(roc)
vasanth:
I think this is better going straight to 2.0 since that's where this is required by the launching partner.
1.4 does not need this.
Flags: needinfo?(vasanth)
(Assignee)

Comment 2

4 years ago
Created attachment 8472906 [details] [diff] [review]
AudioOffloadPlayer-avoid-startup-glitch.patch

Updated patch sets mFallbackToStateMachine to true in one more error case.
Assignee: nobody → vasanth
Attachment #8472321 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8472321 - Flags: review?(roc)
Attachment #8472906 - Flags: review?(roc)
(Assignee)

Comment 3

4 years ago
(In reply to Wayne Chang [:wchang] from comment #1)
> vasanth:
> I think this is better going straight to 2.0 since that's where this is
> required by the launching partner.
> 1.4 does not need this.

Customer raised this issue to us on 1.4 
Moving ni to him
Flags: needinfo?(jaemin1.song)
(Assignee)

Updated

4 years ago
Flags: needinfo?(vasanth)
(v2.0 would be where this is needed ultimately, we don't really need this to land in upstream v1.4)
blocking-b2g: 1.4? → 2.0?
Flags: needinfo?(jaemin1.song)
blocking-b2g: 2.0? → 2.0+
Flags: in-moztrap?(ychung)
No STR is present to create test case to address bug.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)

Updated

4 years ago
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage?][2.0-signoff-need+]
(Assignee)

Comment 6

4 years ago
(In reply to Yeojin Chung [:YeojinC] from comment #5)
> No STR is present to create test case to address bug.

1. In MSM platforms (Ex: Flame, QRD8x10) play any .mp3 music file.
2. It would result in offload playback (From logcat, AuidoFlinger will show compress-offload-playback logs. Not sure if there is any other way to find this)
3. Without this patch we can hear a minor glitch during song start up. You can spot the difference by playing same clip with this path or in VLC.

Let me know if you need more info.

(In reply to Michael Vines [:m1] [:evilmachines] from comment #4)
> (v2.0 would be where this is needed ultimately, we don't really need this to
> land in upstream v1.4)

Thanks! Will upload a patch for 2.0
Test case added in moztrap:

https://moztrap.mozilla.org/manage/case/14339/
QA Whiteboard: [QAnalyst-Triage?][2.0-signoff-need+] → [QAnalyst-Triage+][2.0-signoff-need+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(ychung)
Flags: in-moztrap+
(Assignee)

Comment 8

4 years ago
Created attachment 8474524 [details] [diff] [review]
AudioOffloadPlayer-avoid-startup-glitch-V2.0.patch
Attachment #8472906 - Attachment is obsolete: true
Attachment #8474524 - Flags: review+
(Assignee)

Comment 9

4 years ago
Created attachment 8474525 [details] [diff] [review]
AudioOffloadPlayer-avoid-startup-glitch-V2.1.patch

Rebased the patch for V2.0 AND V2.1
Attachment #8474525 - Flags: review+
(Assignee)

Comment 11

4 years ago
(In reply to vasanth from comment #10)
> 2.1 - https://tbpl.mozilla.org/?tree=Try&rev=3bdecf4857b0
2.1 try build failed. My bad with using hg.

Started new 2.1 try run https://tbpl.mozilla.org/?tree=Try&rev=8e9d579217a1
(Assignee)

Comment 12

4 years ago
Try runs seems good now. starred failures. Adding checkin-needed
Keywords: checkin-needed
(Assignee)

Comment 13

4 years ago
Patch for Mozilla-Central - AudioOffloadPlayer-avoid-startup-glitch-V2.1.patch
Patch for 2.0 -  AudioOffloadPlayer-avoid-startup-glitch-V2.0.patch
https://hg.mozilla.org/mozilla-central/rev/9f6ea1c97cc6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/2b622e00d27a
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
status-firefox32: --- → wontfix
status-firefox33: --- → wontfix
status-firefox34: --- → fixed
You need to log in before you can comment on or make changes to this bug.