Please check the OMXCodec log as below. It's google's native MP3 decoder, not support 'pause' interface. I/OMXCodec( 324): [OMX.google.mp3.decoder] init begin I/OMXCodec( 324): [OMX.google.mp3.decoder] setState(LOADED_TO_IDLE) ! kRequiresLoadedToIdleAfterAllocation I/OMXCodec( 324): [OMX.google.mp3.decoder] Now Idle. mState(0), mState I/OMXCodec( 324): [OMX.google.mp3.decoder] Now Executing. I/OMXCodec( 324): [OMX.google.mp3.decoder] init done E/OMXCodec( 324): [OMX.google.mp3.decoder] Timed out waiting for output buffers: 4/0 E/OMXCodec( 324): [OMX.google.mp3.decoder] Timed out waiting for output buffers: 4/0 E/OMXCodec( 324): [OMX.google.mp3.decoder] Timed out waiting for output buffers: 4/0 E/OMXCodec( 324): [OMX.google.mp3.decoder] Timed out waiting for output buffers: 4/0 E/OMXCodec( 324): [OMX.google.mp3.decoder] Timed out waiting for output buffers: 4/0 E/OMXCodec( 324): [OMX.google.mp3.decoder] Timed out waiting for output buffers: 4/0

Hi Sotaro, Not all vendors will implement MediaSource ‘pause’ interface. Bug 860760 patch can't work properly with google's native decoder, maybe only works fine with qcom's decoder. Pls help to check this issue, thanks.

Tara's hardware decoder is low power usage, need not to implement pause interface to reduce power usage, like unagi or leo. I suggest that gecko should adapt different hardware.

Thanks for reporting, I am going to change to call pause only on qcom's hw codec.

Hi mvines, if qcom's device use software codec, may has the same issue.

(In reply to James Zhang from comment #2) > Tara's hardware decoder is low power usage, need not to implement pause > interface to reduce power usage, like unagi or leo. I suggest that gecko > should adapt different hardware. Hi James, Before Bug 860760 landed, Gecko just stop to read data from OMXCodec and not do anything when user presses pause in the UI. And the Bug 860760 only added "additional steps" to call OMXCodec::Pause() and OMXCodec::Play() so 1. About calling OmxDecoder::Pause(), even others didn't implement it, it just returned with some kind of error code. So will this effect anything? 2. About OmxDecoder::Play(), it seems to add the additional chance on calling mAudioSource->start(). (originally only OmxDecoder::Init() will call it.) So may I know the issue is on "calling OMXCodec::Pause()" or "second time of calling OMXCodec::Start()"? Thanks.

By the way, I tested playing mp3 by SW codec - OMX.google.mp3.decoder and there is no issue. So is this really a Leo+ bug. Or the issue is appeared on the BSP provided by other chip vendor. Hi Sotaro, May I know your opinion on comment 5 & 6? Thanks.

(In reply to Marco Chen [:mchen] from comment #6) > By the way, I tested playing mp3 by SW codec - OMX.google.mp3.decoder and > there is no issue. > So is this really a Leo+ bug. Or the issue is appeared on the BSP provided > by other chip vendor. > > Hi Sotaro, > > May I know your opinion on comment 5 & 6? Thanks. Play and pause twice，then play，the music will be stuck.

(In reply to James Zhang from comment #7) > Play and pause twice，then play，the music will be stuck. First, I see the log as below so make sure that I used SW codec to play. E/OMXCodec( 1620): Attempting to allocate OMX node 'OMX.google.mp3.decoder' E/OMXCodec( 1620): Successfully allocated OMX node 'OMX.google.mp3.decoder' And I performed transition between play <-> pause, I didn't see any issue there.

Maybe Sotaro has fixed this issue. I modify uangi 13.05.30 weekly build code to use kSoftwareCodecsOnly, and unagi works fine now, it can't work properly in unagi 13.05.02 weekly build with kSoftwareCodecsOnly.

(In reply to James Zhang from comment #9) Thanks for you to double confirm this question. Then may I know any other issue here if ::pause() is working well on soft codec too?

Bug 864180 - Move audio software decoder to app's process. r=doublec, a=leo+ This commit fix this issue.

(In reply to James Zhang from comment #11) > Bug 864180 - Move audio software decoder to app's process. r=doublec, a=leo+ > This commit fix this issue. If the problem was already fixed. Can we close this bug?

From Bug 874165 comment #17 and Bug 874165 comment #18, it is better to analyze more about how OMXCodec works in codec other than qualcomm's one.

Hi James, In AOSP, OMXCodec::pause() just set mPause = true. And OMXCodec::start() will check "mState != LOADED". Mapping that to our code flow now: calling OMXCodec::pause() -> no error is returned. calling OMXCodec::start() -> UNKNOWN_ERROR is returned but we didn't see it as error. calling OMXCodec::read() -> It seems this function will set mPause to false with seek option. So maybe FxOS need to give a fake seek option in read() for focring mPause to false. Is this what you want? Thanks.

Current OMXCodec does not have a problem, because caf's OMXCodec alreay modified the OMXCodec::pause() from AOSP. So, leo device has no problem about this bug. I am going to remove "leo+" flag.

But there is still a problem that OmXCode::pause() do not work correctly with AOSP.

(In reply to Sotaro Ikeda [:sotaro] from comment #16) > But there is still a problem that OmXCode::pause() do not work correctly > with AOSP. There is no good way to detect if OMXCodec is CAF or AOSP.

(In reply to Sotaro Ikeda [:sotaro] from comment #17) > (In reply to Sotaro Ikeda [:sotaro] from comment #16) > > But there is still a problem that OmXCode::pause() do not work correctly > > with AOSP. > > There is no good way to detect if OMXCodec is CAF or AOSP. what about adding moz.omx.use_caf to product.mk, like moz.omx.hw.max_width

Hi Sotaro, I added a property called "ro.moz.omx.customized" which will contain a string "CAF" or others. 1. It will be used to decide whether we want to call OMXDecoder::Pause(). 2. The default value of property is "CAF" so there is not extra functions needed to do by current products. 3. And it needs other chip vendor to add this property.

I am not fun of adding platform name in this case. How about "moz.omx.disable_pause"? Current all products uses this capability. Enable by default is better, I think.

(In reply to Marco Chen [:mchen] from comment #19) > 1. It will be used to decide whether we want to call OMXDecoder::Pause(). > 2. The default value of property is "CAF" so there is not extra functions > needed to do by current products. > 3. And it needs other chip vendor to add this property. Is there a reason to add vendor name?

(In reply to Sotaro Ikeda [:sotaro] from comment #21) > Is there a reason to add vendor name? Hi Sotaro, Thanks for your quick response. The reason for using "CAF" are 1. CAF is not a chip vendor name and it is a open source repository too. 2. My thought is that we need a way to distinguish the different "platform" just like "gonk, coco ...etc" so we can manipulate more then one difference on the code based on different platform. For this issue, it really can be decided by boolean value of "moz.omx.disable_pause". Hi Michael, May I know that any way we can distinguish what platform we used now so we can go different code flow? (In CAF it needs to call OMXCodec::Pause() then :: Start() but not in AOSP.)

(In reply to Marco Chen [:mchen] from comment #22) > (In reply to Sotaro Ikeda [:sotaro] from comment #21) > > Is there a reason to add vendor name? > > Hi Sotaro, > > Thanks for your quick response. > The reason for using "CAF" are > 1. CAF is not a chip vendor name and it is a open source repository too. > 2. My thought is that we need a way to distinguish the different > "platform" just like "gonk, coco ...etc" so we can manipulate more then one > difference on the code based on different platform. IIRC, CAF is actually controlled by qcom. Is it correct? And this property is going to divide gonk to multiple sub platforms. It is what I am considering about. And this property is only for omx.

(In reply to Sotaro Ikeda [:sotaro] from comment #23) > IIRC, CAF is actually controlled by qcom. Is it correct? No. CAF is a project of the Linux Foundation. See http://www.linuxfoundation.org/collaborative-projects/code-aurora-forum

(In reply to Sotaro Ikeda [:sotaro] from comment #23) > (In reply to Marco Chen [:mchen] from comment #22) > > IIRC, CAF is actually controlled by qcom. Is it correct? And this property > is going to divide gonk to multiple sub platforms. It is what I am > considering about. And this property is only for omx. I think we should define a property based on capability not by platform. We already have a name gonk for android platform.

Ok, currently we only find this difference between CAF and others so I will update the next patch with "moz.omx.pause_disabled" and default value will be false. thanks.

:mchen, I assigned you to this bug, because you are working on it. Is it OK?

(In reply to Sotaro Ikeda [:sotaro] from comment #27) > :mchen, I assigned you to this bug, because you are working on it. Is it OK? Yes, thanks for your suggestion too.

Comment on attachment 764195 [details] [diff] [review] Patch v2 Issue: AOSP didn't give implementation on OMXCodec::Pause() and not define OMXCodec::Start() should be called for resuming decoding. The source from CAF was customized to this scenario so it effects the other ones who follow the AOSP. Solution: To add a property called "moz.omx.pause_disabled" with default value as "". Once this property is set to "true" then OmxDecoder will ignore the calling of OMXCodec::Pause() and Start(). According to default value is null, this patch didn't effect the current projects but need others chip vendor to add this property if they didn't support it.

Hi James, Could you help to verify this solution can meet your requirement? Thanks.

Comment on attachment 764195 [details] [diff] [review] Patch v2 When pause isn't implemented, it returns an error code. We should use that error code to switch to a fallback code path. See http://androidxref.com/4.0.4/xref/frameworks/base/include/media/stagefright/MediaSource.h#99

(In reply to Marco Chen [:mchen] from comment #31) > Hi James, > > Could you help to verify this solution can meet your requirement? Thanks. Hi Marco, Yes, thank you. (In reply to Michael Wu [:mwu] from comment #32) > Comment on attachment 764195 [details] [diff] [review] > Patch v2 > > When pause isn't implemented, it returns an error code. We should use that > error code to switch to a fallback code path. > > See > http://androidxref.com/4.0.4/xref/frameworks/base/include/media/stagefright/ > MediaSource.h#99 Hi Michael, We considered this solution, but it seems strange, should call 'pause' Interface first to get the result.

Comment on attachment 764602 [details] [diff] [review] Patch v3 There are two issues here. 1. The call flow of OMXCodec::Pause() -> OMXCodec::Start() is not defined by AOSP. And the re-entry of OMXCodec::Start() can cause to return error code. Solved in this patch by: If chip vendor didn't support OMXCodec::Pause() then it needs to return error code then OMXCodec::Start() will not be called after Pause(). 2. If chip vendor implemented OMXCodec::Pause() and expected OMXCodec::Read() with seek option for resuming then it is still not supported by this patch. After discussing with :mwu, it will be solved until we really meet it.

Comment on attachment 764602 [details] [diff] [review] Patch v3 Review of attachment 764602 [details] [diff] [review]: ----------------------------------------------------------------- r-. Will re-evaluate depending on discussion of video/audio pause comment. ::: content/media/omx/OmxDecoder.cpp @@ +725,5 @@ > if (mPaused) { > return; > } > + > + if (mVideoSource.get() && mVideoSource->pause()) { I think this and the audio check below should check using the error defines so it's obvious that it's returning on an error. Or put a comment before it saying 'pause' is non-zero on error. @@ +735,1 @@ > } What happens if video 'pause' succeeds but audio pause fails? Then the video stream will never be unpaused. Is this likely to happen? Do we need a separate mVideoPaused and mAudioPaused?

(In reply to Chris Double (:doublec) from comment #36) > > + if (mVideoSource.get() && mVideoSource->pause()) { > > I think this and the audio check below should check using the error defines > so it's obvious that it's returning on an error. Or put a comment before it > saying 'pause' is non-zero on error. Ok, will add the comment in next patch. > @@ +735,1 @@ > > } > > What happens if video 'pause' succeeds but audio pause fails? Then the video > stream will never be unpaused. Is this likely to happen? Do we need a > separate mVideoPaused and mAudioPaused? Theoretically, it is likely to happen because there are two OMXComponents for audio & video separately. And also one may be HW decoding and the other one would be SW decoding. I think it is worth to do this and it didn't consume too many additional computing and memory usage.

Please refer to comment 37 for the update of this patch. >> so it's obvious that it's returning on an error. Or put a comment before it In order to align the style in ::Start(), I modified to check return value not add additional comment. Thanks for the review.

Comment on attachment 765174 [details] [diff] [review] Patch v5 Review of attachment 765174 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/omx/OmxDecoder.cpp @@ +710,2 @@ > > + if (mAudioPaused && mAudioSource.get()&& mAudioSource->start() != OK) { Add space between .get() and &&.

Hi James, Could you confirm that this patch is working for you? Thanks.

(In reply to Marco Chen [:mchen] from comment #41) > Hi James, > > Could you confirm that this patch is working for you? > > Thanks. It works fine.

1. Add reviewer name. 2. Re-base to newest code.