Closed Bug 1183788 Opened 6 years ago Closed 4 years ago

Broken MP4/H.264 video playback

Categories

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

41 Branch
ARM
Android
defect
Not set
major

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: esawin, Assigned: droeh)

References

Details

Attachments

(2 files, 6 obsolete files)

Attached file 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
Attached patch Proposed patch (obsolete) — Splinter Review
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.
Flags: needinfo?(esawin)
Attachment #8635049 - Flags: review?(snorp)
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
Attachment #8635049 - Flags: review?(snorp) → review-
I confirm that this fixes the crash, the videos play fine.
Flags: needinfo?(esawin)
Attached patch Proposed patch (updated) (obsolete) — Splinter Review
"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
Attachment #8635049 - Attachment is obsolete: true
Flags: needinfo?(esawin)
Attachment #8636207 - Flags: review?(snorp)
This one looks good, too.
Flags: needinfo?(esawin)
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
Attachment #8636207 - Flags: review?(snorp) → review-
Attached patch Proposed patch (updated) (obsolete) — Splinter Review
This makes the suggested changes, but the way I had to handle GLContext-dependent stuff is a bit ugly I think.
Attachment #8636207 - Attachment is obsolete: true
Attachment #8639912 - Flags: review?(snorp)
Attached patch Proposed patch (updated) (obsolete) — Splinter Review
Attachment #8640032 - Flags: review?(snorp)
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.
Attachment #8640032 - Flags: review?(snorp) → review+
Attachment #8639912 - Attachment is obsolete: true
Attachment #8639912 - Flags: review?(snorp)
Attached patch Proposed patch (updated) (obsolete) — Splinter Review
Carry over snorp's r+
Attachment #8640032 - Attachment is obsolete: true
Attachment #8640647 - Flags: review+
Keywords: checkin-needed
Please provide a Try link.
Keywords: checkin-needed
Attached patch Proposed patch (updated) (obsolete) — Splinter Review
Carry over snorp's r+.
Attachment #8640647 - Attachment is obsolete: true
Attachment #8643051 - Flags: review+
Keywords: checkin-needed
Attached patch is empty.
Keywords: checkin-needed
Exported wrong patch last time; this should do it.
Attachment #8643051 - Attachment is obsolete: true
Attachment #8643313 - Flags: review+
Keywords: checkin-needed
Bug 1322650 removed the related code shown in the crashing stack so I guess this bug is no longer relevant.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.