860760 - [Music] Power consumption is always the same ,irrespective of music paused or played out

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Diego Wilson [:diego]]

1. Title :  When Music is paused, power consumed is always more or less the same
2. Precondition : have a Couple of music files 
3. Tester's Action : play MP3 -> stop MP3-> power key press (lcd, backlight turn off) -> measure sleep current
4. Detailed Symptom (ENG.) : When music is paused and LCD screen set to OFF , Sleep current almost remains the same.
5. Gaia Revision #:   " a78ebf426840b5ef08c0cc3e437ad30aba3e2528 "
6. Expected : When music is not played out, power consumption should be comparatively less
7.Reproducibility: Y
1)Frequency Rate : 100%
8.Comparison Results : 
	1)Model Comparing : 
9. Attached files: 
	1)Log : 
	2)Test Contents : 
	3)Video file :

Comment 1

[Kai-Chih Hu [:khu]]

I am thinking that how about the CPU usage when the music is paused? My device is out of battery now. Will try it later.

Comment 2

[Diego Wilson [:diego]]

Attachment: Pause OMX media sources when playback is paused - v2

OMXCodec needs to now when the stream is paused. Otherwise it will prevent the device from lowering its power state when idle.

Comment 3

[Diego Wilson [:diego]]

Attachment: Pause OMX media sources when playback is paused - mozilla-b2g18 - v2

For landing on mozilla-b2g18 we need to pull in OnDecodeThreadFinish() and OnDecodeThreadStart() to nsBuiltinDecoderStateMachine from the trunk

Comment 4

[Sotaro Ikeda [:sotaro]]

Comment on attachment 736403 [details] [diff] [review]
Pause OMX media sources when playback is paused - v2

looks good.

Comment 5

[Sotaro Ikeda [:sotaro]]

Comment on attachment 736406 [details] [diff] [review]
Pause OMX media sources when playback is paused - mozilla-b2g18 - v2

looks good.

Comment 6

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

(leo+, blocks a blocker)

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/0d95450aee43

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d95450aee43

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/1735961cbdb4

Comment 10

[cheng.luo]

(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> Comment on attachment 736406 [details] [diff] [review]
> Pause OMX media sources when playback is paused - mozilla-b2g18 - v2
> 
> looks good.

There is a hidden trouble. the pause() is not a pure virtual interface like stop().
Not of all Decoders has been implement it. the Decoder would be crashed when resume, because of start() reentry.
what is better suggestion?

crash stack as follow:
 0  libc.so!__libc_android_abort [abort.c : 82 + 0x0]
 1  liblog.so!__android_log_assert [logd_write.c : 246 + 0x3]
 2  libstagefright.so!android::AACSPRDDecoder::start [AACSPRDDecoder.cpp : 140 + 0xf]
 3  libxul.so!android::OmxDecoder::Play [OmxDecoder.cpp : 693 + 0x7]
 4  libxul.so!nsMediaOmxReader::OnDecodeThreadStart [nsMediaOmxReader.cpp : 337 + 0x3]
Or
0  libc.so!__libc_android_abort [abort.c : 82 + 0x0]
 1  liblog.so!__android_log_assert [logd_write.c : 246 + 0x3]
 2  libstagefright.so!android::MP3Decoder::start [MP3SPRDDecoder.cpp : 82 + 0xf]
 3  libxul.so!android::OmxDecoder::Play [OmxDecoder.cpp : 693 + 0x7]
 4  libxul.so!nsMediaOmxReader::OnDecodeThreadStart [nsMediaOmxReader.cpp : 337 + 0x3]

Comment 11

[James Zhang (Spreadtrum)]

Gecko need test 'pause' interface result, because not all vendors implement this interface.

Comment 12

[Sotaro Ikeda [:sotaro]]

Thanks for reporting. Can you file a new bug for it?

Comment 13

[Sotaro Ikeda [:sotaro]]

(In reply to cheng.luo from comment #10)
> crash stack as follow:
>  0  libc.so!__libc_android_abort [abort.c : 82 + 0x0]
>  1  liblog.so!__android_log_assert [logd_write.c : 246 + 0x3]
>  2  libstagefright.so!android::AACSPRDDecoder::start [AACSPRDDecoder.cpp :
> 140 + 0xf]
>  3  libxul.so!android::OmxDecoder::Play [OmxDecoder.cpp : 693 + 0x7]
>  4  libxul.so!nsMediaOmxReader::OnDecodeThreadStart [nsMediaOmxReader.cpp :
> 337 + 0x3]
> Or
> 0  libc.so!__libc_android_abort [abort.c : 82 + 0x0]
>  1  liblog.so!__android_log_assert [logd_write.c : 246 + 0x3]
>  2  libstagefright.so!android::MP3Decoder::start [MP3SPRDDecoder.cpp : 82 +
> 0xf]
>  3  libxul.so!android::OmxDecoder::Play [OmxDecoder.cpp : 693 + 0x7]
>  4  libxul.so!nsMediaOmxReader::OnDecodeThreadStart [nsMediaOmxReader.cpp :
> 337 + 0x3]

The above call stack is not what I expected. Why OmxDecoder::Play() calls MP3Decoder::start()/AACSPRDDecoder::start()? OmxDecoder::Play() should call OMXCodec::start(). It is the android ICS's default implementation. The above stacks seems like pre ICS era. Firefox OS v1.01 and v1.1 expect ICS.

Comment 14

[cheng.luo]

(In reply to Sotaro Ikeda [:sotaro] from comment #13)
> OmxDecoder::Play() should call
> OMXCodec::start(). It is the android ICS's default implementation. The above
> stacks seems like pre ICS era. Firefox OS v1.01 and v1.1 expect ICS.

It seems you are right after discussing with our Decoder developer. the MP3Decoder
and AACSPRDDecoder inherited from pre ICS version. With OMX.google.mp3.decoder and OMX.google.aac.decoder which are google's raw Decoder, it could run OK，Although the sound quality is not very good。
We will adapt it to ICS,but have a question：Is ICS's default implementation of OMXCodec::pause() enough？Is it necessary to add OMX_StatePause for OMXCodec?

Comment 15

[James Zhang (Spreadtrum)]

If use google native MP3/AAC decoder, play/pause/play/pause/play, music will stuck.
Pls check the OMXCodec log as below.

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

I'll file a new bug.

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

Looking at the implementation of OMXCodec::pause, I can't understand how it is related to power consumption for it just set mPaused to true.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

Btw, this bug might have something to do with the timeouts in Bug 916135.
Please check https://bugzilla.mozilla.org/show_bug.cgi?id=916135#c19, root cause #3.

"Implementation of OMXCodec::pause is broken. Calling OMXCodec::start later will return error and can't leave the paused state."

Comment 18

[Diego Wilson [:diego]]

(In reply to JW Wang[:jwwang] from comment #16)
> Looking at the implementation of OMXCodec::pause, I can't understand how it
> is related to power consumption for it just set mPaused to true.

The OMXCodec:pause() is ICS does a bit more than that [1]. Certain codecs go into a lower power mode when it's called.

[1] http://smaug.qualcomm.com:8080/source/xref/b2g_ics_1.2/frameworks/base/media/libstagefright/OMXCodec.cpp#6150

Comment 19

[Diego Wilson [:diego]]

Sorry, wrong link:

https://www.codeaurora.org/cgit/quic/la/platform/frameworks/base/tree/media/libstagefright/OMXCodec.cpp?h=b2g_ics_1.2#n6150

Comment 20

[JW Wang [:jwwang] [:jw_wang]]

The piece of code seems to be vendor-specific. I can't find it in the original Android codebase.

http://androidxref.com/4.0.3_r1/xref/frameworks/base/media/libstagefright/OMXCodec.cpp#4622
http://androidxref.com/4.0.4/xref/frameworks/base/media/libstagefright/OMXCodec.cpp#4622

Comment 21

[Marco Chen [:mchen]]

As I remember that Fugu device already help to return non-supported in OMXCodec::Pause() so Gecko wouldn't call OMXCodec::Start() later.

So we can close this discussion.