1183788 - Broken MP4/H.264 video playback

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: Audio/Video, defect)

Description

[Eugen Sawin [:esawin]]

Attachment: stack-trace.txt

Playing H.264 videos crashes debug builds (see stack trace) on Nexus 5 Android 5.1.1.

Video examples: http://me73.com/media

Comment 1

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch

This makes sure the "gfx.SurfaceTexture.detach.enabled" preference is only checked from the main thread and eliminates unnecessary calls to updateCanDetach.

Eugen, I couldn't reproduce your crash, so could you take a look at this whenever you have a chance? Thanks.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8635049 [details] [diff] [review]
Proposed patch

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

::: gfx/gl/AndroidSurfaceTexture.cpp
@@ +124,5 @@
>    return NS_OK;
>  }
>  
>  void
>  AndroidSurfaceTexture::UpdateCanDetach()

I think we should change this to InitCanDetach() now since we only call it once in Init()

@@ +132,4 @@
>  
> +  if (!sCanDetachInited) {
> +    nsCOMPtr<nsIRunnable> getPref = NS_NewRunnableFunction([&sCanDetach]() {
> +      sCanDetach = Preferences::GetBool("gfx.SurfaceTexture.detach.enabled", true);

This is wrong because we're going to use sCanDetach before this ever returns, and then never use the value again.

@@ +137,5 @@
> +    NS_DispatchToMainThread(getPref);
> +    sCanDetachInited = true;
> +  }
> +
> +    // The API for attach/detach only exists on 16+, and PowerVR has some sort of

Fix indentation here

Comment 3

[Eugen Sawin [:esawin]]

I confirm that this fixes the crash, the videos play fine.

Comment 4

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch (updated)

"gfx.SurfaceTexture.detach.enabled" is now read when gfxPlatform::Init is called to ensure it is read on the main thread; AndroidSurfaceTexture::InitCanDetach gets the value from gfxPlatform.

Eugen, would you mind testing this again whenever you have a moment? Thanks

Comment 5

[Eugen Sawin [:esawin]]

This one looks good, too.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8636207 [details] [diff] [review]
Proposed patch (updated)

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

::: gfx/gl/AndroidSurfaceTexture.cpp
@@ +125,5 @@
>    return NS_OK;
>  }
>  
>  void
> +AndroidSurfaceTexture::InitCanDetach()

We need to just remove this method entirely and do all of the necessary logic in gfxPlatform

::: gfx/thebes/gfxPlatform.h
@@ +633,5 @@
>  
> +    /**
> +     * Used to check whether surface texture detach is enabled.
> +     */
> +    bool CanDetachTextures();

I think the name needs to specifically reference SurfaceTexture. Maybe CanSurfaceTextureDetach() or CanDetachSurfaceTexture()

@@ +735,5 @@
>  
> +    /**
> +     * Initialize mCanDetachTextures from preferences
> +     */
> +    void InitCanDetachTextures();

Do we need this? Can we just do it in Init()?

@@ +761,5 @@
>  
>      mozilla::RefPtr<mozilla::gfx::DrawEventRecorder> mRecorder;
>      mozilla::RefPtr<mozilla::gl::SkiaGLGlue> mSkiaGlue;
> +
> +    // Whether or not texture detach is enabled in preferences.

The comment should indicate that we also take into consideration the Android version and GPU

Comment 7

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch (updated)

This makes the suggested changes, but the way I had to handle GLContext-dependent stuff is a bit ugly I think.

Comment 8

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch (updated)

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8640032 [details] [diff] [review]
Proposed patch (updated)

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

You need to use MOZ_WIDGET_ANDROID in gfxPlatform.cpp, otherwise looks good.

Comment 10

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch (updated)

Carry over snorp's r+

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please provide a Try link.

Comment 12

[Dylan Roeh (:droeh) (he/him)]

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

Comment 13

[Dylan Roeh (:droeh) (he/him)]

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

Comment 14

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch (updated)

Carry over snorp's r+.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Attached patch is empty.

Comment 16

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch (updated)

Exported wrong patch last time; this should do it.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ee70eb03b02

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android crashes (at least x86 at the time of this comment, maybe more as results come in).
https://treeherder.mozilla.org/logviewer.html#?job_id=12521737&repo=mozilla-inbound

https://treeherder.mozilla.org/logviewer.html#?job_id=12521737&repo=mozilla-inbound

Comment 19

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a866b4de644

Comment 20

[Dylan Roeh (:droeh) (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60edebe786cc

Comment 21

[Dylan Roeh (:droeh) (he/him)]

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

Comment 22

[Dylan Roeh (:droeh) (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cc6cb3419cf

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Any update here?

Comment 25

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

Bug 1322650 removed the related code shown in the crashing stack so I guess this bug is no longer relevant.