919590 - H.264 video playback stop on nexus4 soon after starting playback

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Sotaro Ikeda [:sotaro]]

Sometimes h.264 video playback stopped soon after starting video playback. It sometimes happens on browser app first time since phone power on.

I confirmed the problem on v1.2 and master nexus-4 by using the following url.
http://people.mozilla.org/~cpeterson/videos
/H264_Baseline_Profile_Level_30_640x360p.mp4

pre-requisite: Bug 911548

Comment 1

[Sotaro Ikeda [:sotaro]]

This problem happens also in following STR

[1] Start video playback on video app (local video playback)
[2] Push home button (return to home screen)
[3] Tap video app
[4] Start video again by tapping "play" UI.

Almost cases, after [4] video playback stopped soon after start playing.

Comment 2

[Sotaro Ikeda [:sotaro]]

It became clear that this problem caused by calling OMXCodec::pause(). By disabling the calling of OMXCodec::pause() locally, the problem is fixed.

The intent of the API is different from android and current b2g.
- android aosp: OMXCodec::pause() is used to stop handling input data during seeking. It is used for quick seeking. The pause state is local to OMXCodec, it does not make any effect to OMX IL component's state. To exit the pause state, need to call OMXCodec::read() with seek option.
- b2g(b2g caf): OMXCodec::pause() is used to save power consumption on hw codec. The function call change the OMX IL component state to OMX_StatePause. To exit pause state, need to call OMXCodec::start().

Comment 3

[Sotaro Ikeda [:sotaro]]

change to "1.3?". The problem happens only on aosp OMXCodec, then the problem does not happen on b2g caf.

Comment 4

[Sotaro Ikeda [:sotaro]]

Bug 872462 is related bug.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - detect pause support in OmxDecoder

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - add ro.moz.moz.omx.disable_pause property

Comment 7

[Sotaro Ikeda [:sotaro]]

By applying attachment 817942 [details] [diff] [review] and attachment 817944 [details] [diff] [review], the problem was fixed. Confirmed on master nexus-4.

Comment 8

[Sotaro Ikeda [:sotaro]]

Comment on attachment 817942 [details] [diff] [review]
patch part 1 - detect pause support in OmxDecoder

mwu, can I have a feed back about the patches?

Comment 9

[Marco Chen [:mchen]]

Hi Sotaro,

This kind of patch is discussed on https://bugzilla.mozilla.org/show_bug.cgi?id=872462#c32 and c33.
The comment there asked OMXCodec::Pause to return error code if it didn't support the "real pause".

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 ver 2 - detect pause support in OmxDecoder

Apply Comment 9.

Comment 11

[amitav.anand]

Attachment: fix_919590issue.patch

Tested the fix on emulator. The issue is still reproducible. Made some tweaks on the patch. The attached patch(fix_919590issue.patch) works fine.

The change in device.mk file to set the ro.moz.moz.omx.disable_pause=true is required.

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to amitav.anand from comment #11)
> Created attachment 8335179 [details] [diff] [review]
> fix_919590issue.patch
> 

Oh, I made a mistake to falsely revert the logic.

> if (!mIsPauseDisabled)

Comment 13

[Sotaro Ikeda [:sotaro]]

(In reply to amitav.anand from comment #11)
> Created attachment 8335179 [details] [diff] [review]
> fix_919590issue.patch
> 
Thanks for the patch.

Comment 14

[Sotaro Ikeda [:sotaro]]

(In reply to amitav.anand from comment #11)
> Created attachment 8335179 [details] [diff] [review]
> fix_919590issue.patch

It would be great if you create a patch with mozilla's style. The following link as info about it.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

Comment 15

[amitav.anand]

Attachment: Gecko-Bug-919590-H.264-video-playback-stop-on-nexus4-soon-.patch

https://github.com/amitav-anand/gecko-dev/commit/6ce1fe059540dc345eee13e9aad71bcfb112204a

Comment 16

[amitav.anand]

Attachment: device-Bug-919590-H.264-video-playback-stop-on-nexus4-soon-.patch

https://github.com/amitav-anand/android_device_lge_mako/commit/5ccac777b5f9585b192bdbb0eff266ab8ab67fa1

Comment 17

[amitav.anand]

Hi, Sotaro, mwu
Can you please review the patch?

Comment 18

[Jason Smith [:jsmith]]

Sotaro - Is this problem specific to Nexus 4 or will it happen with any phone that we're intending to support JB on? Trying to figure out if this will impact any of our target 1.3 JB supported devices or not.

Comment 19

[Sotaro Ikeda [:sotaro]]

(In reply to Jason Smith [:jsmith] from comment #18)
> Sotaro - Is this problem specific to Nexus 4 or will it happen with any
> phone that we're intending to support JB on? Trying to figure out if this
> will impact any of our target 1.3 JB supported devices or not.

It is specific to AOSP android. It does not affect to product b2g phones, they use caf. CAF does not have a problem.

Comment 20

[Jason Smith [:jsmith]]

Okay. Sounds like this isn't a blocker then.

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #19)
> (In reply to Jason Smith [:jsmith] from comment #18)
> > Sotaro - Is this problem specific to Nexus 4 or will it happen with any
> > phone that we're intending to support JB on? Trying to figure out if this
> > will impact any of our target 1.3 JB supported devices or not.
> 
> It is specific to AOSP android. It does not affect to product b2g phones,
> they use caf. CAF does not have a problem.

Sorry, I forgot about flatfish. flatfish might have a problem.
mchen, do you know if flatfish need this bug's fix?

Comment 22

[Marco Chen [:mchen]]

(In reply to Sotaro Ikeda [:sotaro] from comment #21)
Hi Sotaro,

Currently Fugu and Flatfish have this problem and

  1. Fugu: Vendor helps to return "not ok" from OMXCodec::Pause().

  2. Flatfish: Consider to modify on OMXCodec::Pause() too.

-----------------------

Since Gecko will stop the decoding thread while in pause state, OMXCodec::Read() will not be called. Then we don't need the original usage from OMXCodec::Pause(). 

In case of Flatfish, I think the property is useful for those chip vendor used AOSP directly for libstagefright.

Comment 23

[Marco Chen [:mchen]]

Hi Sotaro,

Another idea is that 

  We reclaim the usage of OMXCodec::Pause() as to support low power mode and resume by OMXCodec::Start().
  If the platform didn't support this then just return "ERROR_UNSUPPORTED". Because we don't need the original design 
  between "OMXCodec::Pause() and OMXCodec::Seek()".

May I know your suggestion?

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to Marco Chen [:mchen] from comment #22)
> Since Gecko will stop the decoding thread while in pause state,
> OMXCodec::Read() will not be called. Then we don't need the original usage
> from OMXCodec::Pause(). 

IIRC, OMXCodec::Pause() is used for setting omx IL component state to paused state. What is your intent of the question?

Comment 25

[Sotaro Ikeda [:sotaro]]

>   We reclaim the usage of OMXCodec::Pause() as to support low power mode and
> resume by OMXCodec::Start().
>   If the platform didn't support this then just return "ERROR_UNSUPPORTED".
> Because we don't need the original design 
>   between "OMXCodec::Pause() and OMXCodec::Seek()".

If gecko do this, aosp default OMXCodec can not be supported.  If we use current way of solution, the above 'return "ERROR_UNSUPPORTED"' is not necessary. Can you explain more?

Comment 26

[Marco Chen [:mchen]]

Currently Gecko flow:

  1. pause from media element.
  2. ...
  3. OMXCodec::Pause() is called
  4. ...
  5. play from media element.
  6. ...
  7. OMXCodec::Start() is called.

Whether step 7 will be performed depends on the return value of step 3.
If we asked OMXCodec who didn't support low power mode to directly return "non ok" from Pause() (also not set mPause to true) then 

  1. we will not call step 7.
  2. the next play can be performed successfully because OMXCodec::mPause didn't be set to TRUE.

Comment 27

[Sotaro Ikeda [:sotaro]]

> Whether step 7 will be performed depends on the return value of step 3.
> If we asked OMXCodec who didn't support low power mode to directly return
> "non ok" from Pause() (also not set mPause to true) then 

As in Comment 25, If b2g require to return error when OMXCodec::pause() when OMXCodec does not support caf type pause, OmxDecoder can not work correctly without modification to OMXCodec. I do not want to modify gonk code if it is not necessary. I want to keep as much as possible to compatible to aosp.

Comment 28

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #24)
> (In reply to Marco Chen [:mchen] from comment #22)
> > Since Gecko will stop the decoding thread while in pause state,
> > OMXCodec::Read() will not be called. Then we don't need the original usage
> > from OMXCodec::Pause(). 
> 
> IIRC, OMXCodec::Pause() is used for setting omx IL component state to paused
> state. What is your intent of the question?

mchen, Can you answer the above question?

Comment 29

[Marco Chen [:mchen]]

Hi Sotaro,

1. could you help to confirm what I understood for OMXCodec::Pause() is right or not?

  a. The only thing to do by OMXCodec::Pause() is set mPause to TRUE.
  b. Then in drainInputBuffers(), it will be returned according to mPause is TRUE.
  c. So this can prevent OMXCodec from continue to reading data from source.

2. Could you confirm what I understood for FxOS media playback's pause state?
  a. The decode thread will be removed when pause state.
  b. So OmxDecoder::ReadVideo/Audio() will not be called during pause state.

3. According to FxOS didn't call readVideo/Audio during pause state so we didn't even have necessary to call OMXCodec::Pause().  

4. For power saving on ascertain platform, FxOS called OMXCodec::Pause() for asking H/W Codec to do power saving but not expect the original benefit from AOSP's OMXCodec::Pause().

5. So we can return "non-ok" from any vendor's OMXCodec::Pause() if it just contain the benefit from AOSP's OMXCodec::Pause() which we didn't need to FxOS.

Comment 30

[Sotaro Ikeda [:sotaro]]

(In reply to Marco Chen [:mchen] from comment #29)
> Hi Sotaro,
> 
> 1. could you help to confirm what I understood for OMXCodec::Pause() is
> right or not?
> 
>   a. The only thing to do by OMXCodec::Pause() is set mPause to TRUE.

In APSP OMXCodec, it is correct. But in b2g caf OMXCodec, Pause() set omx il component to pause state. The state change is necessary to save power consumption on qcom hw.
https://www.codeaurora.org/cgit/quic/la/platform/frameworks/base/tree/media/libstagefright/OMXCodec.cpp?h=b2g_ics_1.2#n6150

>   b. Then in drainInputBuffers(), it will be returned according to mPause is
> TRUE.
>   c. So this can prevent OMXCodec from continue to reading data from source.

It is true only in AOSP OMXCodec, not in b2g caf OMXCodec. In b2g caf OMXCodec case, data handle is stopped because of the omx il component state change.

> 2. Could you confirm what I understood for FxOS media playback's pause state?
>   a. The decode thread will be removed when pause state.

Decode thread will be removed when decode buffers are full and playback is paused.
http://mxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderStateMachine.cpp#2237
http://mxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderStateMachine.cpp#2259

>   b. So OmxDecoder::ReadVideo/Audio() will not be called during pause state.

If decode buffers are not full, ReadVideo/Audio() is called even in pause state.

> 3. According to FxOS didn't call readVideo/Audio during pause state so we
> didn't even have necessary to call OMXCodec::Pause().  

As described above, OMXCodec::Pause() is necessary in qcom hw to supress power consumption.

> 4. For power saving on ascertain platform, FxOS called OMXCodec::Pause() for
> asking H/W Codec to do power saving but not expect the original benefit from
> AOSP's OMXCodec::Pause().

Yes.

> 5. So we can return "non-ok" from any vendor's OMXCodec::Pause() if it just
> contain the benefit from AOSP's OMXCodec::Pause() which we didn't need to
> FxOS.

As I described in Comment 27, I do not want to change gonk source code if it is not necessary. I want to keep the change to codes under ./framework as minimum as possible to keep the source compatibility to android. It is my preference. This problem can be fixed without change to ./framework.

Comment 31

[Sotaro Ikeda [:sotaro]]

OMXCodec is sync API, it causes the some problems during http video playback. Therefore, I am thinking to add support of MediaCodec/Acodec if gonk is more than JB. So, current way of doing is going to be replaced by new one in near future. Even codeaurora need to rethink about it.

Comment 32

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #31)
> OMXCodec is sync API, it causes the some problems during http video
> playback. Therefore, I am thinking to add support of MediaCodec/Acodec if
> gonk is more than JB. So, current way of doing is going to be replaced by
> new one in near future. Even codeaurora need to rethink about it.

In this case, it seems better to implement as aosp codec works without the related modification.

Comment 33

[Sotaro Ikeda [:sotaro]]

Where to handle the error is just preference. If we could get a consensus about it, handle at OMXCodec::Pause() is also OK. To think about Comment 32, handle at OMXCodec::Pause() becomes the problem simpler.

Comment 34

[Marco Chen [:mchen]]

Hi Sotaro,

We are on the evaluation state on what's the exact benefit from ACodec, so we will decide to jump in or not.
If you already have data, could you update it into Bug 904177 so product manager can take it into consideration? Thanks.

-----------------

As I knew that Fugu already took the modification in their OMXCodec::Pause().

Comment 35

[Sotaro Ikeda [:sotaro]]

From Comment 33 and Comment 34, it might be better to modify OMXCodec::Pause() for non b2g caf OMXCodec. cancel the feed back to the patches.

Comment 36

[Sotaro Ikeda [:sotaro]]

b2g does not use aosp OMXCodec::Pause(), it could regress http video seek performance. In aosp, during video seek, audio codec's OMXCodec::Pause() is called to stop data read. In http streaming video case, http channel bandwidth and media cache size is limited. If audio data read is not stopped, audio data read disrupt video data read. In particular, android's media cahec(NuCachedSource2) is continuous buffer, the disruption could become huge. Therefore, OMXCodec::Pause() is very important for android stagefright.

In b2g, media cache do not need to be continuous, therefore impact of the disruption is not big as android. But audio codec continue to read audio data, even during video seek read. This affect to video seek read's performance.

Comment 37

[Sotaro Ikeda [:sotaro]]

In b2g, OMXCodec::Pause() is already used for different purpose. If we move to MediaCodec/Acodec in future b2g, it's better to fix the video seek problem by new MediaCodec/Acodec usage, I think.

Comment 38

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Change aosp OMXCodec::Pause() as to return ERROR_UNSUPPORTED

Comment 39

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8340486 [details] [diff] [review]
patch - Change aosp OMXCodec::Pause() as to return ERROR_UNSUPPORTED

mwu, can I have a feedback from you?

Comment 40

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8340486 [details] [diff] [review]
patch - Change aosp OMXCodec::Pause() as to return ERROR_UNSUPPORTED

mwu, can you review the patch?

Comment 41

[Marco Chen [:mchen]]

Hi Dietrich,

I just reset the needinfo flag in order to notify mwu again. Thanks.

Comment 42

[Sotaro Ikeda [:sotaro]]

Attachment: link to pull request

Replace the patch by the pull request.

Comment 43

[Sotaro Ikeda [:sotaro]]

https://github.com/mozilla-b2g/platform_frameworks_av/commit/0d93be33c0ee226c7bf7be0d7d591981c57fa96f

Comment 44

[Sotaro Ikeda [:sotaro]]

https://github.com/mozilla-b2g/platform_frameworks_av/commit/ee814270f52127febfcf29bacf9f1b327d7c4c29

Comment 45

[Chris Pearce [:cpearce (Not reading bugmail)]]

Sotaro: does this mean we can remove MediaOmxReader::OnDecodeThreadStart() and MediaOmxReader::OnDecodeThreadFinish() ?

Comment 46

[Sotaro Ikeda [:sotaro]]

CAF b2g OMXCodec still needs them. This patch is for default AOSP OMXCodec.

Comment 47

[Bruce Sun [:brsun]]

Just for your information. RtspOmxReader, which inherits from MediaOmxReader, needs OnDecodeThreadStart() and OnDecodeThreadFinish().