1222923 - App crashed when try to replay video on nexus-5-l

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Alison Shiue]

Attachment: TV_fling_player_crashed.log

I have ever met this crash issue 3 times, however I don't have specific reproduce steps, but it happens easily when I tried to replay cast video from TV.

Build info:
Build ID               20151108150206
Gaia Revision          23cab7ea0fcecab7689d340baf604e024e88f9a3
Gaia Date              2015-11-09 06:13:17
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/e2a910c048dc82fc3be53475f18e7f81f03e377b
Gecko Version          45.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20151108.182159
Firmware Date          Sun Nov  8 18:22:17 EST 2015
Bootloader             HHZ12f

Comment 1

[Alison Shiue]

Attachment: 624da3c6-69e2-45e3-6d4ae771-663f73cb.dmp

Comment 2

[Alison Shiue]

[Blocking Requested - why for this release]:
crash issue

Comment 3

[Josh Cheng [:josh]]

Hi James, Hi SC,
Could you help to check this issue?
Thanks!

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Found following trace during log inspection, need to find out what causes the mozalloc_abort:

>D/OmxDecoder( 3167): Unexpected error when seeking to 0
>D/OmxDecoder( 3167): SeekTime: 10000000, mLastSeekTime:0
>D/OmxDecoder( 3167): Unexpected error when seeking to 10000000
>D/OmxDecoder( 3167): SeekTime: 20000000, mLastSeekTime:10000000
>E/Gecko   ( 3167): mozalloc_abort: Redirecting call to abort() to mozalloc_abort
>E/qdhwcomposer(  196): Display is blanked - Cannot enable vsync
>E/ANDR-PERF-LOCK(  884): Failed to reset optimization for resource: 4 level: 0
>D/qdhwcomposer(  196): hwc_blank: Unblanking display: 0
>D/qdhwcomposer(  196): hwc_blank: Done unblanking display: 0
>I/Gecko   (  196): 
>I/Gecko   (  196): ###!!! [Parent][MessageChannel] Error: (msgtype=0x280080,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
>I/Gecko   (  196):

Comment 5

[James Cheng[:JamesCheng]]

Hi,
Sorry for the delay response.
I cannot reproduce the crash issue...but I find another crash issue and posted on Bug 1224113.
I will still keep reproducing this issue.

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: crash_stack.log

This is the crash stack converted from attachment 8684807 [details], symbol files can be downloaded [1].

It seems caused by an assertion in OMXCodec::onCmdComplete [2], which is in Android codebase. @bwu can you find someone to take look at this crash stack? I'm not sure if this crash is something we can fix on B2G.

[1] from https://tools.taskcluster.net/index/artifacts/#gecko.v2.mozilla-central.revision.e2a910c048dc82fc3be53475f18e7f81f03e377b.b2g/gecko.v2.mozilla-central.revision.e2a910c048dc82fc3be53475f18e7f81f03e377b.b2g.nexus-5-l-eng-opt
[2] https://android.googlesource.com/platform/frameworks/av/+/android-cts-5.1_r4/media/libstagefright/OMXCodec.cpp#2589

Comment 7

[Blake Wu [:bwu][:blakewu]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #6)
> Created attachment 8687798 [details]
> crash_stack.log
> 
> This is the crash stack converted from attachment 8684807 [details], symbol
> files can be downloaded [1].
> 
> It seems caused by an assertion in OMXCodec::onCmdComplete [2], which is in
> Android codebase. @bwu can you find someone to take look at this crash
> stack? I'm not sure if this crash is something we can fix on B2G.
Except MP3, most formats are decoded via MediaCodec instead of OMXCodec. It looks strange unless it was playing mp3 files. 
Alison, 
I don't quite understand how you tested it in comment 0. Could you record a video to show me?
And what is the content you were using to test?
Thanks!

Comment 8

[Alison Shiue]

mp4: http://people.mozilla.org/~schien/demo.mp4

The crashed video: https://youtu.be/krRjm8qEsHg
(fling player crashed at 00:21)

Build info:
Build ID               20151116150206
Gaia Revision          6c761e221b8563e1ea6d39b60581d25b6aaae206
Gaia Date              2015-11-16 19:40:37
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/d1cae7deae1ac0aa4c2a4b1aca1f3b6966fa58b4
Gecko Version          45.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20151116.181129
Firmware Date          Mon Nov 16 18:11:48 EST 2015
Bootloader             HHZ12f

Comment 9

[Blake Wu [:bwu][:blakewu]]

Alison,
Thanks for your effort to record a video to show us.:)
Could you also help test to just play that video file on your "TV" directly?
That will help us narrow down the problem. 
Thanks!

Comment 10

[Josh Cheng [:josh]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #9)
> Alison,
> Thanks for your effort to record a video to show us.:)
> Could you also help test to just play that video file on your "TV" directly?
> That will help us narrow down the problem. 
> Thanks!

Hi Blake,
There is currently no video player on smart TV. What else do you think could help here?
Thanks!

Comment 11

[Blake Wu [:bwu][:blakewu]]

oh. I think I misunderstood. so it is a wifi display/miracast?

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

The fling player is just a app uses <video> and provide customized control UI, which is using JS to do video play/pause/seek. From the video provided by @ashuie, it just some button click that triggers video seek to end and play from beginning.

I really don't think doing comment #9 can do any help on this issue. @bwu, please guide QA to turn on any log you think will help or teach them to setup a debugging environment before you ask them to reproduce this issue.

Or you can discuss with me directly to figure out how to debug this issue.

Comment 13

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

BTW, I think we can slightly modify fling app to play video directly without using Fennec to launch the webapp. It should provide a simpler environment for debugging and reproducing.

Comment 14

[Blake Wu [:bwu][:blakewu]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #12)
> The fling player is just a app uses <video> and provide customized control
> UI, which is using JS to do video play/pause/seek. From the video provided
> by @ashuie, it just some button click that triggers video seek to end and
> play from beginning.
> 
> I really don't think doing comment #9 can do any help on this issue. @bwu,
> please guide QA to turn on any log you think will help or teach them to
> setup a debugging environment before you ask them to reproduce this issue.
> 
> Or you can discuss with me directly to figure out how to debug this issue.
Thanks for your feedback. I don't quite understand what magic behind this fling player and the whole cast thing. Let me discuss with you offline first. :)

Comment 15

[James Cheng[:JamesCheng]]

Attachment: fling-player-played-video-directly-without-Fennec.patch

Upload a patch that enable fling player playing video directly without Fennec provided by @Fischer Liu

Comment 16

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: debug.patch

Thanks @JamesCheng! I rebase your patch to latest gaia master branch.

Hi @bwu, you can apply this patch on latest Gaia source and simply use |make GAIA_DEVICE_TYPE=tv reset-gaia| in gaia folder to flash the TV UI on to your nexus-5.

BTW, TV UI use swipe gesture to emulate directional key navigation, e.g. swipe up means pressing ArrowUpKey. Double click emulates EnterKey. QA will be happy to help you if you don't know how to use the TV UI.

Comment 17

[Sotaro Ikeda [:sotaro]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #7)
> > 
> > It seems caused by an assertion in OMXCodec::onCmdComplete [2], which is in
> > Android codebase. @bwu can you find someone to take look at this crash
> > stack? I'm not sure if this crash is something we can fix on B2G.
> Except MP3, most formats are decoded via MediaCodec instead of OMXCodec. It
> looks strange unless it was playing mp3 files. 
> Alison, 
> I don't quite understand how you tested it in comment 0. Could you record a
> video to show me?
> And what is the content you were using to test?
> Thanks!

Yea, on b2g v2.5 and b2g master, for mp4 video playback, OMXCodec should not be used. It means preference setting seems wrong.

OMXCodec means, it uses MediaOmxReader for mp4 video playback. But it should not work on b2g v2.5 and b2g master. MediaFormatReader should be used for mp4 playback, it uses MediaCodec for decoding.

Comment 18

[Sotaro Ikeda [:sotaro]]

It might be helpful to write STR clearly in the bug.

Comment 19

[Sotaro Ikeda [:sotaro]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #6)
> Created attachment 8687798 [details]
> crash_stack.log
> 
> This is the crash stack converted from attachment 8684807 [details], symbol
> files can be downloaded [1].
> 
> It seems caused by an assertion in OMXCodec::onCmdComplete [2], which is in
> Android codebase. @bwu can you find someone to take look at this crash
> stack? I'm not sure if this crash is something we can fix on B2G.
> 
> [1] from
> https://tools.taskcluster.net/index/artifacts/#gecko.v2.mozilla-central.
> revision.e2a910c048dc82fc3be53475f18e7f81f03e377b.b2g/gecko.v2.mozilla-
> central.revision.e2a910c048dc82fc3be53475f18e7f81f03e377b.b2g.nexus-5-l-eng-
> opt
> [2]
> https://android.googlesource.com/platform/frameworks/av/+/android-cts-5.1_r4/
> media/libstagefright/OMXCodec.cpp#2589

:schien, in the bug's environment, what kind of gonk source is used for testing? On mozbuild nexus-5-l, we do not use aosp source directly. It uses the following code.

https://github.com/sotaroikeda/platform_frameworks_av/blob/b2g-5.1.0_r1/media/libstagefright/OMXCodec.cpp#L2589

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Oh, I didn't know that we have fork the libstagefright. QA uses the nexus-5-l build downloaded from TaskCluster. The source code you provide should be the correct one.

BTW, in comment #17 you mentioned there is some preference for configuring playback. However, the test device in this bug is only overriding Gaia with TV UI. Can you provide the preference name so that we can double check if this bug is related to the test device setup.

Comment 21

[Sotaro Ikeda [:sotaro]]

It is "media.mp4.enabled". It is enabled by default. Therefore, the problem might be caused by other factors. It seems better to check if MediaFormatReader and MP4Decoder is always created to playback mp4 files on devices.
  https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/MP4Decoder.cpp#181
  https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#320

MP4Decoder::CanHandleMediaType() actually checks if gecko could use MP4Decoder for playback.
 https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/MP4Decoder.cpp#93

InstantiateDecoder() creates MP4Decoder.
  https://dxr.mozilla.org/mozilla-central/source/dom/media/DecoderTraits.cpp#592

Comment 22

[Sotaro Ikeda [:sotaro]]

(In reply to Alison Shiue from comment #8)
> mp4: http://people.mozilla.org/~schien/demo.mp4
> 

I just tested the video on v2.5 nexus-5-l by using ROM that I build by myself. I confirmed that the video playback uses OmxDecoder. I am going to look into it.

Comment 23

[Sotaro Ikeda [:sotaro]]

On gonk L, MOZ_FMP4 seems not defined.

Comment 24

[Sotaro Ikeda [:sotaro]]

The following seems to have a problem
 https://dxr.mozilla.org/mozilla-central/source/configure.in#280

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Change to configure.in

Comment 26

[Blake Wu [:bwu][:blakewu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #22)
> (In reply to Alison Shiue from comment #8)
> > mp4: http://people.mozilla.org/~schien/demo.mp4
> > 
> 
> I just tested the video on v2.5 nexus-5-l by using ROM that I build by
> myself. I confirmed that the video playback uses OmxDecoder. I am going to
> look into it.
Sotaro, 
Thanks for your help to look into!

Comment 27

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - GonkVideoDecoderManager

Comment 28

[Blake Wu [:bwu][:blakewu]]

(In reply to Josh Cheng [:josh] from comment #3)
> Hi James, Hi SC,
> Could you help to check this issue?
> Thanks!
Josh, 
This crash only happens on Android decoding pipeline. AFAIK, our TV partner uses their decoding pipeline, so this should not happen on our partner's TV and this is not a blocker for them.

Comment 29

[Sotaro Ikeda [:sotaro]]

By applying attachment 8689332 [details] [diff] [review] and attachment 8689357 [details] [diff] [review]. I confirmed that MediaFormatReader is used for the video playback. But it seems trigger another defect. The video has green bar on the bottom.

Comment 30

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Thanks to @sotaro and @bwu! So this issue is actually a playback issue on Android L, which is not ready for b2g v2.5. Update the title and corresponding fields to reflect the reality.

To @ctang and @ashiue, we should switch to nexus-5 KK to avoid these kind of Android L specific issue.

Comment 31

[John Lin [:jhlin][:jolin]]

Comment on attachment 8689357 [details] [diff] [review]
patch part 2 - GonkVideoDecoderManager

Review of attachment 8689357 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot!

Comment 32

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - GonkVideoDecoderManager

Remove unnecessary defined(MOZ_WIDGET_GONK). Carry "r=jolin".

Comment 33

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f6b7ebf1674

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d44a046927f6

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 36

[Jean-Yves Avenard [:jya]]

Glancing at the patch, mNativeWindow may not always be initialised in Init() and it's no longer set to nullptr in the constructor. 

Yet, it only check for mNeedsCopyBuffer being false to use mNativeWindow making it possible to use mNativeWindow at random value.

Comment 37

[John Lin [:jhlin][:jolin]]

(In reply to Jean-Yves Avenard [:jya] from comment #36)
> Glancing at the patch, mNativeWindow may not always be initialised in Init()
> and it's no longer set to nullptr in the constructor. 
> 
> Yet, it only check for mNeedsCopyBuffer being false to use mNativeWindow
> making it possible to use mNativeWindow at random value.

 mNativeWindow is an android::sp<> whose default constructor initializes the pointer with 0 [1]. I guess that's why sotaro thinks it's okay to be removed from the initialization list.

[1] http://androidxref.com/4.4_r1/xref/system/core/include/utils/StrongPointer.h#61

Comment 38

[Jean-Yves Avenard [:jya]]

My main concern is more about this:
https://hg.mozilla.org/mozilla-central/file/d44a046927f6/dom/media/platforms/gonk/GonkVideoDecoderManager.cpp#l381

mNativeWindow is used if mNeedsCopyBuffer is false, and it's false by default.

There's no test that mNativeWindows isn't nullptr here so there's potentially a null deref.

Comment 39

[John Lin [:jhlin][:jolin]]

(In reply to Jean-Yves Avenard [:jya] from comment #38)
> My main concern is more about this:
> https://hg.mozilla.org/mozilla-central/file/d44a046927f6/dom/media/platforms/
> gonk/GonkVideoDecoderManager.cpp#l381
> 
> mNativeWindow is used if mNeedsCopyBuffer is false, and it's false by
> default.
> 
> There's no test that mNativeWindows isn't nullptr here so there's
> potentially a null deref.

 The check at [1] implicitly guarantees mNativeWindow is not nullptr. MediaCodecProxy returns graphic buffers only when we give it a surface derived from mNativeWindow at [2]. 
 
[1] https://hg.mozilla.org/mozilla-central/file/d44a046927f6/dom/media/platforms/gonk/GonkVideoDecoderManager.cpp#l198
[2] https://hg.mozilla.org/mozilla-central/file/d44a046927f6/dom/media/platforms/gonk/GonkVideoDecoderManager.cpp#l619

Comment 40

[Jean-Yves Avenard [:jya]]

(In reply to John Lin [:jolin][:jhlin] from comment #39)
>  The check at [1] implicitly guarantees mNativeWindow is not nullptr.
> MediaCodecProxy returns graphic buffers only when we give it a surface
> derived from mNativeWindow at [2]. 

it's the "implicitly" bit I'm concerned about.

It's certainly not guaranteed.
What if it doesn't.

The code assume that some far remote code is doing the right thing. If it's guaranteed, it should be a strong assert.

Comment 41

[Sotaro Ikeda [:sotaro]]

(In reply to Jean-Yves Avenard [:jya] from comment #36)
> Glancing at the patch, mNativeWindow may not always be initialised in Init()
> and it's no longer set to nullptr in the constructor. 

It is not necessary to set to nullptr in the constructor. androd::sp initialize it as to refer to nullptr.