Video corruption with Galaxy s3 (SCH i535)

RESOLVED FIXED in Firefox 53

Status

()

Firefox for Android
Audio/Video
P1
normal
RESOLVED FIXED
29 days ago
19 days ago

People

(Reporter: kbrosnan, Assigned: JamesCheng)

Tracking

(Blocks: 1 bug, {regression})

Trunk
Firefox 55
ARM
Android
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53blocking fixed, firefox54+ fixed, firefox55+ fixed, fennec53+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

29 days ago
Created attachment 8862934 [details]
Example of video corruption

Seeing a few reports of corrupted video. One user has a North American Galaxy s3 (SCH i535)
(Reporter)

Comment 1

26 days ago
Another report mentions the Galaxy S4.
John, Blake, can you take a look or help find an owner for this bug? Thanks.
tracking-firefox53: ? → +
tracking-firefox54: ? → +
tracking-firefox55: ? → +
Flags: needinfo?(jolin)
Flags: needinfo?(bwu)
Kevin, was Android version mentioned in the reports? Thanks a lot.
Flags: needinfo?(kbrosnan)
Flags: needinfo?(jolin)
Flags: needinfo?(bwu)
(Reporter)

Comment 4

25 days ago
Samsung Galaxy Tab 4 is another affected device.

This only happens with the North American variant of the phones, Qualcom SOC. The international variants use Samsung Exynos SOCs seem not to reproduce. Android versions are 4.2 - 4.4. It may be easier for someone on Snorp's team to get a NA device.
Flags: needinfo?(kbrosnan)

Comment 5

25 days ago
Tagging as release blocking due to it's severity.
tracking-firefox53: + → blocking

Comment 6

25 days ago
Has anyone given a specific URL?

Comment 7

25 days ago
Never mind. No need for a URL. I see it. I'll see if I can track the regression.
(Reporter)

Comment 8

25 days ago
Mike is working on a regression range as he has hardware. If there is a good backout candidate we may take that.
Flags: needinfo?(mozilla)

Comment 9

25 days ago
This was caused by https://bugzil.la/1317239

I have verified it is still happening in current builds.

Mike
Flags: needinfo?(mozilla)
(Reporter)

Comment 10

25 days ago
The full range is https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=47f42f21541b9b98ad7db82edb996b29065debd0&tochange=34fce7c12173bdd6dda54c2ebf6d344252f1ac48

Mike and I agree that 1317239 is the best candidate from that range. There are a couple possibilities

* backout 1317239 - this would remove adaptive streaming from video elements for all devices
* require Android 5 (API 21) for the feature
* blacklist some SOC/GPU combinations
Blocks: 1317239
Flags: needinfo?(jacheng)
Keywords: regressionwindow-wanted
Note that this is holding up 53.0.2 which we need quickly for a partner preload issue, so we'd like to get an answer ASAP.
Looks like Chromium had met similar issues before: https://codereview.chromium.org/1869103002
Comment hidden (mozreview-request)
(Assignee)

Updated

24 days ago
Attachment #8863993 - Flags: review?(jolin)
(Assignee)

Comment 14

24 days ago
Hi Mike, 

Since I don't have Samsung Galaxy S3 or any infected devices, I can't confirm this symptom will be solved by this patch.

I would like to do

1. Let this patch be landed in nightly.

2. Need your help to confirm this issue is solved on Galaxy S3.

3. Request uplifting to beta.

Thanks.
Flags: needinfo?(jacheng)

Comment 15

24 days ago
mozreview-review
Comment on attachment 8863993 [details]
Bug 1360626 - create a blacklist for adaptive playback support.

https://reviewboard.mozilla.org/r/135718/#review138732

::: commit-message-a748a:1
(Diff revision 1)
> +Bug 1360626 - Blacklisted the adaptive playback support.

Nit: 'create a blacklist for adaptive playback support'

::: commit-message-a748a:2
(Diff revision 1)
> +Bug 1360626 - Blacklisted the adaptive playback support.
> +

Describe briefly why blacklisting is needed.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/HardwareCodecCapabilityUtils.java:67
(Diff revision 1)
>        }
>      }
>      return false;
>    }
>  
> +  // See Bug1360626 and

Nit: put this method below `checkSupportsAdaptivePlayback()`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/HardwareCodecCapabilityUtils.java:83
(Diff revision 1)
> +    if (!Build.MANUFACTURER.toLowerCase(Locale.getDefault()).equals("samsung")) {
> +      return false;
> +    }
> +
> +    return Build.MODEL.startsWith("GT-I9300") || // S3 (I9300 / I9300I)
> +           Build.MODEL.startsWith("SCH-I535"); // S3

Galaxy S4 was also mentioned in Kevin's bug comment, perhaps we should list that too?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/HardwareCodecCapabilityUtils.java:89
(Diff revision 1)
> +  }
> +
>    @WrapForJNI
>    public static boolean checkSupportsAdaptivePlayback(MediaCodec aCodec, String aMimeType) {
>        // isFeatureSupported supported on API level >= 19.
>        if (!(Build.VERSION.SDK_INT >= 19)) {

Nit:
`if (Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT || isAdaptivePlaybackBlacklisted(aMimeType))`
Attachment #8863993 - Flags: review?(jolin) → review+
(Assignee)

Comment 16

24 days ago
(In reply to John Lin [:jolin][:jhlin] from comment #15)
> Comment on attachment 8863993 [details]
> Bug 1360626 - Blacklisted the adaptive playback support.
> 
> https://reviewboard.mozilla.org/r/135718/#review138732
> 
> ::: commit-message-a748a:1

> mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/
> HardwareCodecCapabilityUtils.java:83
> (Diff revision 1)
> > +    if (!Build.MANUFACTURER.toLowerCase(Locale.getDefault()).equals("samsung")) {
> > +      return false;
> > +    }
> > +
> > +    return Build.MODEL.startsWith("GT-I9300") || // S3 (I9300 / I9300I)
> > +           Build.MODEL.startsWith("SCH-I535"); // S3
> 
> Galaxy S4 was also mentioned in Kevin's bug comment, perhaps we should list
> that too?

Hi Kevin,

Since the chromium code didn't consider Galaxy S4 for this case,

Could you please double confirm if these symptom really happened on S4 and provide me some detail information about the android version and SOC info?

Since S4 https://en.wikipedia.org/wiki/Samsung_Galaxy_S4#Model_variants used various of SOC model for different countries, it is hard for us to blacklist all the infected devices.

Thanks.
Flags: needinfo?(kbrosnan)
Comment hidden (mozreview-request)
(Assignee)

Updated

24 days ago
Assignee: nobody → jacheng
Comment hidden (mozreview-request)
(Assignee)

Comment 19

24 days ago
Hi Mike and Kevin,

I've pushed try and the apk which includes the patch can be downloaded from

https://queue.taskcluster.net/v1/task/RKGWvzY6StWn0jHxzn9A1A/runs/0/artifacts/public/build/target.apk

Would you please install the app and help to check if the symptom can be solved by this APK on S3?

For S4, I expect the symptom can still be reproduced and also need Kevin's information about the device Model for S4.


Thank you.
Flags: needinfo?(mozilla)
tracking-fennec: ? → 53+
Priority: -- → P1
Yes, the APK you posted fixes the problem on my S3.
Flags: needinfo?(mozilla)
This is a really specific blacklist (and it blacklists all S3).

Is this what Google did?
(Reporter)

Comment 22

24 days ago
These are user reports mostly without model numbers. 

https://support.mozilla.org/en-US/questions/1157981
https://support.mozilla.org/en-US/questions/1158597
https://support.mozilla.org/en-US/questions/1158437 <- claims to run a S4
https://support.mozilla.org/en-US/questions/1158516 <- claims to run a Galaxy Tab 4
https://support.mozilla.org/en-US/questions/1158443
https://www.reddit.com/r/firefox/comments/67oxvz/squiggly_grey_box_video_on_video_sites_like/
Flags: needinfo?(kbrosnan)
It looks like there a couple nits on the patch. James, can you clean that up and land it when you get in? The release folks want to spin a dot release ASAP.
Flags: needinfo?(jacheng)
(Assignee)

Comment 24

23 days ago
Hi snorp, Mike,

I've fixed the nits as John mentioned but my patch only handled Galaxy S3 cases as Google did in [1]

Do you agree that I land it first and create a follow up bug to enlarge the blacklist to cover some sorts of S4 or S4 tab cases?

Since https://en.wikipedia.org/wiki/Samsung_Galaxy_S4#Model_variants has various type of chips that I want to forbidden the failure ones instead of all types of model.


[1]
https://codereview.chromium.org/1869103002/patch/40001/50004
Flags: needinfo?(jacheng) → needinfo?(snorp)
Comment hidden (mozreview-request)

Comment 26

23 days ago
mozreview-review
Comment on attachment 8863993 [details]
Bug 1360626 - create a blacklist for adaptive playback support.

https://reviewboard.mozilla.org/r/135718/#review138750

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/HardwareCodecCapabilityUtils.java:102
(Diff revisions 3 - 4)
>  
>      if (!Build.MANUFACTURER.toLowerCase(Locale.getDefault()).equals("samsung")) {
>        return false;
>      }
>  
> -    return Build.MODEL.startsWith("GT-I9300") || // S3 (I9300 / I9300I)
> +    final String[] blacklist =

Nit: make this `static` class variable.
Comment hidden (mozreview-request)
Is this still a blocker for 53.0.2?

I'm running up against a hard deadline for a partner, so I was hoping 53.0.2 would spin today.

Snorp: Are you ok with his patch? Wondering if we can push.
I'd also like someone to land this so that I can start the 53.0.2 build as early as possible today.
(In reply to James Cheng[:JamesCheng] from comment #24)
> Hi snorp, Mike,
> 
> I've fixed the nits as John mentioned but my patch only handled Galaxy S3
> cases as Google did in [1]
> 
> Do you agree that I land it first and create a follow up bug to enlarge the
> blacklist to cover some sorts of S4 or S4 tab cases?

I think that's fine. We can land this patch even with the small nit to get 53.0.2 out.
Flags: needinfo?(snorp)

Comment 31

23 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8f8dc26b27eb
Create a blacklist for adaptive playback support. r=jolin
Comment on attachment 8863993 [details]
Bug 1360626 - create a blacklist for adaptive playback support.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1317239
[User impact if declined]: Video corruption on some devices
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Would be helpful. Simply play a video on an affected device such as a Galaxy S3.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It only disables a feature that was only recently added on a subset of devices.
[String changes made/needed]: None
Attachment #8863993 - Flags: approval-mozilla-release?
Attachment #8863993 - Flags: approval-mozilla-beta?
Attachment #8863993 - Flags: approval-mozilla-aurora?
Comment on attachment 8863993 [details]
Bug 1360626 - create a blacklist for adaptive playback support.

Great, let's get this on release and beta. 
We don't need it on aurora now because of Project Dawn.
Attachment #8863993 - Flags: approval-mozilla-release?
Attachment #8863993 - Flags: approval-mozilla-release+
Attachment #8863993 - Flags: approval-mozilla-beta?
Attachment #8863993 - Flags: approval-mozilla-beta+
Attachment #8863993 - Flags: approval-mozilla-aurora?
Attachment #8863993 - Flags: approval-mozilla-aurora-

Comment 34

23 days ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-release/rev/f87a819106bd
status-firefox53: affected → fixed

Comment 35

23 days ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/6022f5353130
status-firefox54: affected → fixed
(Assignee)

Updated

22 days ago
Blocks: 1362271

Comment 36

22 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8f8dc26b27eb
Status: NEW → RESOLVED
Last Resolved: 22 days ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 37

21 days ago
I have this same issue.  Galaxy S4 running Android 4.4.4
Model SGH-M919

Comment 38

20 days ago
Same here. Samsung Galaxy S4 GT-I9505 Android 4.4.2 / https://queue.taskcluster.net/v1/task/RKGWvzY6StWn0jHxzn9A1A/runs/0/artifacts/public/build/target.apk -no change here.
(Assignee)

Updated

19 days ago
Blocks: 1362918
You need to log in before you can comment on or make changes to this bug.