I can reproduce on a Pixel 7 also. And no, it doesn't reproduce on all videos or sites. I can view other videos on YouTube and Reddit without issue (though I've seen this more than once on Reddit now).

Jamie Nicol [:jnicol]

Assignee

Comment 7

•

2 years ago

•

Edited

I can also repro on a Pixel 8 running Android 14, but not a 6a or 7 running Android 13. So the assumption is this affects all Tensor Pixel devices (6, 7, 8; normal, a, Pro)

Interestingly I cannot repro in Chrome normally. However, when you swipe up from the bottom of the screen to open the app switcher, the chrome thumbnail does reproduce the problem. Likewise taking a screenshot of Chrome reproduces the problem.

My hunch is that the video data is in a dataspace not handled by either our gles renderer nor chrome's. However, in normal circumstances I believe chrome uses SurfaceControl to composite the screen, which presumably does handle the dataspace correctly.

Logcat shows this on an affected device:

     CCodecBuffers: [c2.exynos.h264.decoder#888:2D-Output] popFromStashAndRegister: at 0us, output format changed to AMessage(what = 0x00000000) = {
     CCodecBuffers:   int32_t android._color-format = 2130708361
     CCodecBuffers:   int32_t android._video-scaling = 1
     CCodecBuffers:   int32_t rotation-degrees = 0
     CCodecBuffers:   int32_t color-standard = 10
     CCodecBuffers:   int32_t color-range = 2
     CCodecBuffers:   int32_t color-transfer = 3
     CCodecBuffers:   float cta861.max-cll = 0.000000
     CCodecBuffers:   float cta861.max-fall = 0.000000
     CCodecBuffers:   int32_t sar-height = 1
     CCodecBuffers:   int32_t sar-width = 1
     CCodecBuffers:   Rect crop(0, 0, 479, 639)
     CCodecBuffers:   int32_t width = 480
     CCodecBuffers:   int32_t height = 640
     CCodecBuffers:   int32_t max-height = 1080
     CCodecBuffers:   int32_t max-width = 1920
     CCodecBuffers:   string mime = "video/raw"
     CCodecBuffers:   int32_t priority = 0
     CCodecBuffers:   int32_t android._dataspace = 281673728
     CCodecBuffers:   int32_t color-format = 2130708361
     CCodecBuffers: }

And this on an unaffected device:

     CCodec  : setup formats output: AMessage(what = 0x00000000) = {
     CCodec  :   int32_t android._color-format = 2130708361
     CCodec  :   int32_t android._video-scaling = 1
     CCodec  :   int32_t rotation-degrees = 0
     CCodec  :   int32_t color-standard = 4
     CCodec  :   int32_t color-range = 2
     CCodec  :   int32_t color-transfer = 3
     CCodec  :   float cta861.max-cll = 0.000000
     CCodec  :   float cta861.max-fall = 0.000000
     CCodec  :   int32_t sar-height = 1
     CCodec  :   int32_t sar-width = 1
     CCodec  :   Rect crop(0, 0, 95, 127)
     CCodec  :   int32_t width = 96
     CCodec  :   int32_t height = 128
     CCodec  :   int32_t max-height = 1080
     CCodec  :   int32_t max-width = 1920
     CCodec  :   string mime = "video/raw"
     CCodec  :   int32_t priority = 0
     CCodec  :   int32_t android._dataspace = 259
     CCodec  :   int32_t color-format = 2130708361
     CCodec  : }

Interestingly, on unaffected devices color-standard is 4 and android._dataspace is 259. On affected devices they are 10 and 281673728 on affected.

4 is COLOR_STANDARD_BT601_NTSC and 259 appears to be HAL_DATASPACE_V0_BT601_525 . I'm not sure what 10 and 281673728 are.

EDIT: 10 appears to come from here: kColorStandardDCI_P3. Which makes sense given the color_primaries=smpte432 discovery below.

Jamie Nicol [:jnicol]

Assignee

Comment 8

•

2 years ago

I made a very simple app, which decodes the above video using MediaCodec directly in to a SurfaceView. This exhibits the same behaviour as chrome: It renders correctly when the app is foregrounded. But in the app switcher it renders incorrectly. I'm certain that if I instead decode into a SurfaceTexture, then render from that to the SurfaceView using gles, then it will fully reproduce the bug like Firefox does.

In fact, even just leaving the app alone for 10 or so seconds will make it start to render incorrectly. Or pulling down the notification shade half-way, or adjusting the device volume. Any of these actions presumably cause surfaceflinger to use its internal gles compositing path rather than direct scan-out, which is enough to reproduce the bug.

So this definitely isn't a Firefox bug. But it clearly affects us worse than it does Chrome, which is a problem.

Jamie Nicol [:jnicol]

Assignee

Comment 9

•

2 years ago

According to ffprobe, the broken video has color_primaries=smpte432.

Converting that to bt709 with ffmpeg -i bad.mp4 -color_primaries bt709 good.mp4 makes it render correctly.

Likewise converting a previously good video to have smpte432 color_primaries makes it no longer render correctly.

Jamie Nicol [:jnicol]

Assignee

Comment 10

•

2 years ago

Went to file an upstream bug and found someone had already beaten me to it: https://issuetracker.google.com/issues/313499362

I also filed an issue with Chrome as that might give the Android team a bit more urgency to fix it: https://bugs.chromium.org/p/chromium/issues/detail?id=1506942

Ryan VanderMeulen [:RyanVM]

Updated

•

2 years ago

status-firefox120: affected → wontfix

status-firefox121: affected → fix-optional

Jamie Nicol [:jnicol]

Assignee

Comment 11

•

2 years ago

Paul, do you know whether we have any mechanism to fall back to software decoding when we detect a certain color_primaries? (which we can do in HandleOutputFormatChanged()). That might be a good enough solution while we wait for this to be fixed upstream, or find a better workaround.

Flags: needinfo?(padenot)

Paul Adenot (:padenot)

Comment 12

•

2 years ago

I don't think we do, but that would be https://developer.android.com/reference/kotlin/android/media/MediaCodecList, using https://searchfox.org/mozilla-central/search?q=symbol:S_java_java_org%2Fmozilla%2Fgecko%2Fmedia%2FCodec%23SW_CODEC_PREFIX.&redirect=false and https://developer.android.com/reference/kotlin/android/media/MediaCodec#createByCodecName(kotlin.String).

Flags: needinfo?(padenot)

Jamie Nicol [:jnicol]

Assignee

Comment 13

•

2 years ago

I tried forcing a software decoder as suggested, which appears to work, but the bug still occurs.

However, I found the OpenGL extension EXT_YUV_target, which allows us to override the colorspace transformation when sampling from an external texture. This is supported on all the affected Pixel devices. I made a proof of concept app which works here. So will now try to integrate that into webrender.

Jamie Nicol [:jnicol]

Assignee

Comment 14

•

2 years ago

Attached file Bug 1866020 - Override buggy colorspace conversion on Pixel devices. r?gw!,padenot! — Details

Pixel 6, 7, and 8 devices running Android 14 are affected by a bug
where video frames with SMPTE 432 color primaries are rendered
incorrectly when sampled from an external texture in GLES. To work
around this, we force these frames to be converted to RGB using BT709
colorspace instead. While this won't look exactly right, it is much
better than the current situation.

When we detect that a frame is decoded with that color space on an
affected device, we set a "ForceBT709" flag which gets passed through
to webrender as a new ImageBufferKind. Rendering this ImageBufferKind
is handled via a new shader feature TEXTURE_EXTERNAL_BT709, which
works much like the existing TEXTURE_EXTERNAL feature, but
additionally uses the EXT_YUV_TARGET extension to override the
colorspace transformation.

This approach could be extended in the future to handle additional
colorspace transformations, but for now only handles the one required
to workaround this particular driver bug.

Phabricator Automation

Updated

•

2 years ago

Assignee: nobody → jnicol

Status: NEW → ASSIGNED

Pulsebot

Comment 15

•

2 years ago

Pushed by jnicol@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/290917deea87 Override buggy colorspace conversion on Pixel devices. r=gw,padenot,geckoview-reviewers,owlish

Narcis Beleuzu [:NarcisB]

Comment 16

•

2 years ago

Backed out for wrench bustages.

Backout link: https://hg.mozilla.org/integration/autoland/rev/7f65ebc83171b418c7ef679ed1809791721abd74
Log link: https://treeherder.mozilla.org/logviewer?job_id=439928239&repo=autoland&lineNumber=1550

Flags: needinfo?(jnicol)

Pulsebot

Comment 17

•

2 years ago

Pushed by jnicol@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/da24abf34123 Override buggy colorspace conversion on Pixel devices. r=gw,padenot,geckoview-reviewers,owlish

Noemi Erli[:noemi_erli]

Comment 18

•

2 years ago

bugherder

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

Status: ASSIGNED → RESOLVED

Closed: 2 years ago

status-firefox122: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → 122 Branch

Ryan VanderMeulen [:RyanVM]

Updated

•

2 years ago

Flags: needinfo?(jnicol)

Andrew McCreight [:mccr8]

Updated

•

2 years ago

Regressions: 1870413

Ryan VanderMeulen [:RyanVM]

Comment 19

•

1 year ago

I can confirm that this is fixed on my Pixel 7 using the original testcase. Did you want to nominate this and bug 1870413 for release uplift, Jamie?

Flags: needinfo?(jnicol)

Jamie Nicol [:jnicol]

Assignee

Comment 20

•

1 year ago

Attached file Bug 1866020 - Override buggy colorspace conversion on Pixel devices. r?gw!,padenot! — Details

Pixel 6, 7, and 8 devices running Android 14 are affected by a bug
where video frames with SMPTE 432 color primaries are rendered
incorrectly when sampled from an external texture in GLES. To work
around this, we force these frames to be converted to RGB using BT709
colorspace instead. While this won't look exactly right, it is much
better than the current situation.

When we detect that a frame is decoded with that color space on an
affected device, we set a "ForceBT709" flag which gets passed through
to webrender as a new ImageBufferKind. Rendering this ImageBufferKind
is handled via a new shader feature TEXTURE_EXTERNAL_BT709, which
works much like the existing TEXTURE_EXTERNAL feature, but
additionally uses the EXT_YUV_TARGET extension to override the
colorspace transformation.

This approach could be extended in the future to handle additional
colorspace transformations, but for now only handles the one required
to workaround this particular driver bug.

Original Revision: https://phabricator.services.mozilla.com/D195800

Phabricator Automation

Updated

•

1 year ago

Attachment #9369820 - Flags: approval-mozilla-release?

Phabricator Automation

Comment 21

•

1 year ago

Uplift Approval Request

Needs manual QE test: no
Fix verified in Nightly: yes
User impact if declined: Videos with certain colorspace rendered incorrectly on Pixel devices
Explanation of risk level: Simple patch. Workaround only used on affected devices
String changes made/needed: None
Steps to reproduce for manual QE testing: N/A
Code covered by automated testing: yes
Risk associated with taking this patch: Low
Is Android affected?: yes

Jamie Nicol [:jnicol]

Assignee

Comment 22

•

1 year ago

Done

Flags: needinfo?(jnicol)

Lando Automation

Updated

•

1 year ago

status-firefox121: fix-optional → fixed

Pulsebot

Comment 23

•

1 year ago

uplift

https://hg.mozilla.org/releases/mozilla-release/rev/a8024dd0537f

Ryan VanderMeulen [:RyanVM]

Updated

•

1 year ago

Attachment #9369820 - Flags: approval-mozilla-release? → approval-mozilla-release+

Ryan VanderMeulen [:RyanVM]

Updated

•

1 year ago

Blocks: 1879720

Screenshot_20231122-021624.png 2 years ago Kevin Brosnan 1.21 MB, image/png		Details
about-support.txt 2 years ago Kevin Brosnan 19.29 KB, text/plain		Details
Bug 1866020 - Override buggy colorspace conversion on Pixel devices. r?gw!,padenot! 2 years ago Jamie Nicol [:jnicol] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1866020 - Override buggy colorspace conversion on Pixel devices. r?gw!,padenot! 1 year ago Jamie Nicol [:jnicol] 48 bytes, text/x-phabricator-request	RyanVM : approval-mozilla-release+	Details \| Review

video playback looks like a negative image Pixel 6 Pro

Uplift Approval Request