872462 - [tara]music or video playback will stuck if press button to play and pause twice, caused by bug 860760

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[James Zhang (Spreadtrum)]

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

Comment 1

[James Zhang (Spreadtrum)]

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.

Comment 2

[James Zhang (Spreadtrum)]

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.

Comment 3

[Sotaro Ikeda [:sotaro]]

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

Comment 4

[James Zhang (Spreadtrum)]

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

Comment 5

[Marco Chen [:mchen]]

(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.

Comment 6

[Marco Chen [:mchen]]

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.

Comment 7

[James Zhang (Spreadtrum)]

(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.

Comment 8

[Marco Chen [:mchen]]

(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.

Comment 9

[James Zhang (Spreadtrum)]

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.

Comment 10

[Marco Chen [:mchen]]

(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?

Comment 11

[James Zhang (Spreadtrum)]

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

Comment 12

[Sotaro Ikeda [:sotaro]]

(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?

Comment 13

[Sotaro Ikeda [:sotaro]]

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.

Comment 14

[Marco Chen [:mchen]]

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.

Comment 15

[Sotaro Ikeda [:sotaro]]

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.

Comment 16

[Sotaro Ikeda [:sotaro]]

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

Comment 17

[Sotaro Ikeda [:sotaro]]

(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.

Comment 18

[James Zhang (Spreadtrum)]

(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

Comment 19

[Marco Chen [:mchen]]

Attachment: Patch v1

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.

Comment 20

[Sotaro Ikeda [:sotaro]]

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.

Comment 21

[Sotaro Ikeda [:sotaro]]

(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?

Comment 22

[Marco Chen [:mchen]]

(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.)

Comment 23

[Sotaro Ikeda [:sotaro]]

(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.

Comment 24

[Michael Vines [:m1] [:evilmachines]]

(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

Comment 25

[Sotaro Ikeda [:sotaro]]

(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.

Comment 26

[Marco Chen [:mchen]]

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.

Comment 27

[Sotaro Ikeda [:sotaro]]

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

Comment 28

[Marco Chen [:mchen]]

Attachment: Patch v2

Comment 29

[Marco Chen [:mchen]]

(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 30

[Marco Chen [:mchen]]

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.

Comment 31

[Marco Chen [:mchen]]

Hi James,

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

Comment 32

[Michael Wu [:mwu]]

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

Comment 33

[James Zhang (Spreadtrum)]

(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 34

[Marco Chen [:mchen]]

Attachment: Patch v3

Comment 35

[Marco Chen [:mchen]]

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 36

[cajbir (:cajbir)]

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?

Comment 37

[Marco Chen [:mchen]]

(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.

Comment 38

[Marco Chen [:mchen]]

Attachment: Patch v4

Comment 39

[Marco Chen [:mchen]]

Attachment: Patch v5

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 40

[cajbir (:cajbir)]

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 &&.

Comment 41

[Marco Chen [:mchen]]

Hi James,

Could you confirm that this patch is working for you?

Thanks.

Comment 42

[James Zhang (Spreadtrum)]

(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.

Comment 43

[Marco Chen [:mchen]]

https://tbpl.mozilla.org/?tree=Try&rev=d5fbba94d140

Comment 44

[Marco Chen [:mchen]]

Attachment: Patch Checkin Version

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

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/e4852f5f6ce4

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e4852f5f6ce4