Sotaro Ikeda [:sotaro] Assignee
	Description • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 1 • 11 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 2 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 3 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 4 • 11 years ago

Bug 872462 is related bug.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 7 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 8 • 11 years ago

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?

	Marco Chen [:mchen]
	Comment 9 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 10 • 11 years ago

Apply Comment 9.

	amitav.anand
	Comment 11 • 11 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 12 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 13 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 14 • 11 years ago

(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

	amitav.anand
	Comment 15 • 11 years ago

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

	amitav.anand
	Comment 16 • 11 years ago

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

	amitav.anand
	Comment 17 • 11 years ago

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

	Jason Smith [:jsmith]
	Comment 18 • 11 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 19 • 11 years ago

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

	Jason Smith [:jsmith]
	Comment 20 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 21 • 11 years ago

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

	Marco Chen [:mchen]
	Comment 22 • 11 years ago

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

	Marco Chen [:mchen]
	Comment 23 • 11 years ago

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?

	Sotaro Ikeda [:sotaro] Assignee
	Comment 24 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 25 • 11 years ago

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

	Marco Chen [:mchen]
	Comment 26 • 11 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 27 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 28 • 11 years ago

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

	Marco Chen [:mchen]
	Comment 29 • 11 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 30 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 31 • 11 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 32 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 33 • 11 years ago

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.

	Marco Chen [:mchen]
	Comment 34 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 35 • 11 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 36 • 11 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 37 • 11 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 39 • 11 years ago

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?

	Sotaro Ikeda [:sotaro] Assignee
	Comment 40 • 11 years ago

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

mwu, can you review the patch?

	Marco Chen [:mchen]
	Comment 41 • 11 years ago

Hi Dietrich,

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 42 • 11 years ago

Replace the patch by the pull request.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 43 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 44 • 11 years ago

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

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 45 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 46 • 11 years ago

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

	Bruce Sun [:brsun]
	Comment 47 • 11 years ago

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