Closed Bug 940177 Opened 12 years ago Closed 12 years ago

[Audio] there is abnormal sounds or no sound while pressing the dial key very quickly

Categories

(Firefox OS Graveyard :: AudioChannel, defect)

Product:
Firefox OS Graveyard
Component:
AudioChannel
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: scheng, Assigned: scheng)

References

Details

Attachments

(1 file, 2 obsolete files)

Bug-940177.patch-reviewed
1.26 KB, patch
Details | Diff | Splinter Review
Description: When a user press the dial key very quickly. There is abnormal sounds or no sound. Repro Steps: 1) Enter Dial App 2) Press the dial keys very quick Actual: There is no sound or abnormal sound. Expected: There is normal sound for pressing each dial key. Environmental Variables: Device: Buri v1.2 Com RIL BuildID: 20131111004004 Gaia: 670b2c8329bca6f142939185be71274166d82bb8 Gecko: 1ac147e4e2f0 Version: 26.0 Notes: Repro frequency: 100%
Assignee: nobody → scheng
Attached patch Bug-940177.patch (obsolete) — — Splinter Review
Enqueueing the audio pcm data to the opensl buffer and then set the status as playing to avoid insufficient buffer to use.(the OpenSLES warning message : BufferQueue::Enqueue (SL_RESULT_BUFFER_INSUFFICIENT) )
Attachment #8337640 - Flags: review?(mchen)
Comment on attachment 8337640 [details] [diff] [review] Bug-940177.patch Hi Star, Matthew is the owner of libcubed. Hi Matthew, Please help to review this bug. Thanks.
Attachment #8337640 - Flags: review?(mchen) → review?(kinetik)
So, I'm kind of confused here. This patch shouldn't make a difference, according to the OpenSL ES 1.0.1 spec from Khronos: From 8.12 SLBufferQueueItf: "When the media object is in the SL_PLAYSTATE_PLAYING state, which is controlled by the SLPlayItf interface [see section 8.32], adding buffers will implicitly start playback. In the case of starvation due to insufficient buffers in the queue the playing of audio data stops. The media object remains in the SL_PLAYSTATE_PLAYING state. Upon queuing of additional buffers, the playing of audio data resumes. Note that starvation of queued buffers causes audible gaps in the audio data stream. If the maximum number of buffers specified in the SLDataLocator_BufferQueue structure used as the data source when creating the media object using the CreateAudioPlayer or CreateMidiPlayer method has been reached, the buffer is not added to the buffer queue and SL_RESULT_BUFFER_INSUFFICIENT is returned. At this point the client should wait until it receives a callback notification for a buffer completion at which time it can enqueue the buffer." So we should only see SL_RESULT_BUFFER_INSUFFICIENT if we attempt to enqueue more buffers than are available, but that shouldn't be possible because bufferqueue_callback queries the number of buffers in the queue and only querienqueues the free buffers.
Comment on attachment 8337640 [details] [diff] [review] Bug-940177.patch r- until we (or I) understand this better.
Attachment #8337640 - Flags: review?(kinetik) → review-
f? padenot in case he has any thoughts about this.
Flags: needinfo?(paul)
Chrome for Android fill the buffer queue, and the starts the playback. http://mxr.mozilla.org/chromium/source/src/media/audio/android/opensles_output.cc#94 This got me curious, so I ended up reading the implementation of OpenSL we use on B2G. It appears that Android does NOT prefill on start, so we have to do it manually before. But as Matthew said, we should not see SL_RESULT_BUFFER_INSUFFICIENT. I'll investigate.
Flags: needinfo?(paul)
To Connect Bluetooth A2DP, and then play .mp3 file in Music app. That's easy to see the "BufferQueue::Enqueue (SL_RESULT_BUFFER_INSUFFICIENT)" message.
I am sorry about lacking enough information : It should be that play .mp3 file and "seek".
AudioSteam invokes cubeb_stream_start() -> opensl_stream_start(), this function set the PlayState as SL_PLAYSTATE_PLAYING, and the player state machine mechanism will call the call back function(bufferqueue_callback()) when it need aduio data. But bufferqueue_callback() be called in opensl_stream_start() later. There are 2 threads to call the same function, but there is no mutex protection in this function. My Questions: 1.Should we need to call bufferqueue_callback() in opensl_stream_start()? (Because the OPENSL state machine mechanism will call it.) 2.If we need to call it, should we move the bufferqueue_callback() function before as the SL_PLAYSTATE_PLAYING status?
Flags: needinfo?(paul)
Flags: needinfo?(kinetik)
In opensl_stream_start(), I remove "bufferqueue_callback(NULL, stm); " this line. The music player can't work. (player don't play and do nothing.) I will investigate it.
[1] There are 2 threads(1287 & 1289) interleavedly access the same buffer and data. The race condition obviously occurs. The content of "queuebuf_idx" and "state.count" are wrong. 1029 1287 E CUBEB_OPENSL: bufferqueue_callback state.count 0 ith 1 user_ptr 0xb3070d80 queuebuf_idx 1 1029 1287 E CUBEB_OPENSL: [star] bufferqueue_callback written 1103 1029 1287 E CUBEB_OPENSL: bufferqueue_callback state.count 0 ith 2 user_ptr 0xb3070d80 queuebuf_idx 2 1029 1289 E CUBEB_OPENSL: [star] ++++++++++++++++++++++++++++++++++ user_ptr 0xb3070d80 1029 1289 E CUBEB_OPENSL: bufferqueue_callback state.count 1 ith 1 user_ptr 0xb3070d80 queuebuf_idx 2 1029 1287 E CUBEB_OPENSL: [star] bufferqueue_callback written 1103 1029 1287 E CUBEB_OPENSL: bufferqueue_callback state.count 0 ith 3 user_ptr 0xb3070d80 queuebuf_idx 3 1029 1289 E CUBEB_OPENSL: [star] bufferqueue_callback written 1103 1029 1289 E CUBEB_OPENSL: bufferqueue_callback state.count 1 ith 2 user_ptr 0xb3070d80 queuebuf_idx 0 1029 1287 E CUBEB_OPENSL: [star] bufferqueue_callback written 1103 1029 1287 E CUBEB_OPENSL: [star] ------------------------------------ 4 user_ptr 0xb3070d80 1029 1289 E CUBEB_OPENSL: [star] bufferqueue_callback written 1103 1029 1289 W libOpenSLES: Leaving BufferQueue::Enqueue (SL_RESULT_BUFFER_INSUFFICIENT) 1029 1289 E CUBEB_OPENSL: bufferqueue_callback state.count 1 ith 3 user_ptr 0xb3070d80 queuebuf_idx 2 1029 1289 E CUBEB_OPENSL: [star] bufferqueue_callback written 1103 1029 1289 W libOpenSLES: Leaving BufferQueue::Enqueue (SL_RESULT_BUFFER_INSUFFICIENT) 1029 1289 E CUBEB_OPENSL: [star] ------------------------------------ 4 user_ptr 0xb3070d80 1029 1289 E CUBEB_OPENSL: [star] ++++++++++++++++++++++++++++++++++ user_ptr 0xb3070d80 1029 1289 E CUBEB_OPENSL: bufferqueue_callback state.count 3 ith 3 user_ptr 0xb3070d80 queuebuf_idx 3 1029 1289 E CUBEB_OPENSL: [star] bufferqueue_callback written 1103 1029 1289 E CUBEB_OPENSL: [star] ------------------------------------ 4 user_ptr 0xb3070d80 [2] It looks like our expectation. 1064 1192 E CUBEB_OPENSL: [star] ++++++++++++++++++++++++++++++++++ user_ptr 0xb0720b00 1064 1192 E CUBEB_OPENSL: bufferqueue_callback state.count 0 ith 0 user_ptr 0xb0720b00 queuebuf_idx 0 1064 1192 E CUBEB_OPENSL: [star] bufferqueue_callback written 1103 1064 1192 E CUBEB_OPENSL: bufferqueue_callback state.count 0 ith 1 user_ptr 0xb0720b00 queuebuf_idx 1 1064 1192 E CUBEB_OPENSL: [star] bufferqueue_callback written 1103 1064 1192 E CUBEB_OPENSL: bufferqueue_callback state.count 0 ith 2 user_ptr 0xb0720b00 queuebuf_idx 2 1064 1192 E CUBEB_OPENSL: [star] bufferqueue_callback written 1103 1064 1192 E CUBEB_OPENSL: bufferqueue_callback state.count 0 ith 3 user_ptr 0xb0720b00 queuebuf_idx 3 1064 1192 E CUBEB_OPENSL: [star] bufferqueue_callback written 1103 1064 1192 E CUBEB_OPENSL: [star] ------------------------------------ 4 user_ptr 0xb0720b00 1064 1195 E CUBEB_OPENSL: [star] ++++++++++++++++++++++++++++++++++ user_ptr 0xb0720b00 1064 1195 E CUBEB_OPENSL: bufferqueue_callback state.count 3 ith 3 user_ptr 0xb0720b00 queuebuf_idx 0 1064 1195 E CUBEB_OPENSL: [star] bufferqueue_callback written 1103 1064 1195 E CUBEB_OPENSL: [star] ------------------------------------ 4 user_ptr 0xb0720b00 1064 1195 E CUBEB_OPENSL: [star] ++++++++++++++++++++++++++++++++++ user_ptr 0xb0720b00 1064 1195 E CUBEB_OPENSL: bufferqueue_callback state.count 3 ith 3 user_ptr 0xb0720b00 queuebuf_idx 1 1064 1195 E CUBEB_OPENSL: [star] bufferqueue_callback written 1103 1064 1195 E CUBEB_OPENSL: [star] ------------------------------------ 4 user_ptr 0xb0720b00 1064 1195 E CUBEB_OPENSL: [star] ++++++++++++++++++++++++++++++++++ user_ptr 0xb0720b00 1064 1195 E CUBEB_OPENSL: bufferqueue_callback state.count 3 ith 3 user_ptr 0xb0720b00 queuebuf_idx 2 1064 1195 E CUBEB_OPENSL: [star] bufferqueue_callback written 1103 1064 1195 E CUBEB_OPENSL: [star] ------------------------------------ 4 user_ptr 0xb0720b00 1064 1195 E CUBEB_OPENSL: [star] ++++++++++++++++++++++++++++++++++ user_ptr 0xb0720b00 1064 1195 E CUBEB_OPENSL: bufferqueue_callback state.count 3 ith 3 user_ptr 0xb0720b00 queuebuf_idx 3 1064 1195 E CUBEB_OPENSL: [star] bufferqueue_callback written 1103 1064 1195 E CUBEB_OPENSL: [star] ------------------------------------ 4 user_ptr 0xb0720b00 my experiment: I move "bufferqueue_callback(NULL, stm);" before set SL_PLAYSTATE_PLAYING state in the opensl_stream_start() function. The result is case [2]. there is no race condition.
Okay, that makes a lot of sense after all. Thanks for digging, it's obvious in retrospect!
Flags: needinfo?(paul)
Flags: needinfo?(kinetik)
Comment on attachment 8337640 [details] [diff] [review] Bug-940177.patch Let's take this. But please add a comment above bufferqueue_callback saying something like "refill the queues before starting playback to avoid racing with refills started by SetPlayState on OpenSL threads." Thanks for investigating!
Attachment #8337640 - Flags: review- → review+
Attached patch Bug-940177-v2.patch (obsolete) — — Splinter Review
add comment to explain why we move bufferqueue_callback() function before set play state as playing.
Attachment #8337640 - Attachment is obsolete: true
1.push patch to try server - try server result link : https://tbpl.mozilla.org/?tree=Try&rev=95faa55ec694 2.add reviewer (r=kinetik)
Attachment #8351163 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #8356044 - Attachment is patch: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: