Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for 10/12 bits YUV images #3113

Merged
merged 1 commit into from Sep 25, 2018
Merged

Conversation

@jyavenard
Copy link
Contributor

jyavenard commented Sep 24, 2018

We define the ColorDepth type in image as ultimately all types of images should support native color bit depth higher than 8 bits.

This change requires other patches found in bug 1493198


This change is Reviewable

Copy link
Member

kvark left a comment

Looks great in general! Is this blocked on 1493198 changes, or the other way around?
Left a few concerns below

Reviewed 11 of 11 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jyavenard)


webrender_api/src/image.rs, line 142 at r1 (raw file):

    }
    /// Return how many bytes is used to encode a pixel's channel
    pub fn bytes_per_pixel(self) -> u32 {

does it make sense to have a "bytes per channel" metric if the channels can be tightly packed together? i.e. YUV10 should fit in 4 bytes total, but this metric would make it look like it needs 3 * 2 = 6 bytes


webrender_api/src/image.rs, line 154 at r1 (raw file):

        match self {
            ColorDepth::Color8 => 1.0,
            ColorDepth::Color10 => 64.0,

are these the proper constants though? if you have a floating-point value in range 0 - 1, and you encode it in N bits, the scaler is (2^N - 1). So if you want to re-scale it from one representation to another, you'd need (2^N - 1) / (2^M -1) instead of just 2^(N-M). Unless, of course, we are doing something to compensate for this when we fill out the A16 image data.

Copy link
Contributor Author

jyavenard left a comment

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jyavenard)


webrender_api/src/image.rs, line 142 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

does it make sense to have a "bytes per channel" metric if the channels can be tightly packed together? i.e. YUV10 should fit in 4 bytes total, but this metric would make it look like it needs 3 * 2 = 6 bytes

YUV format have many shapes and form, this is really the number of bytes per channel's pixel. The size of a pixel with all components would be for YUV420: width*2+(width/2)*2+(width/2)2=width4

I'll remove the method, it's only needed when feeling the planes.


webrender_api/src/image.rs, line 154 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

are these the proper constants though? if you have a floating-point value in range 0 - 1, and you encode it in N bits, the scaler is (2^N - 1). So if you want to re-scale it from one representation to another, you'd need (2^N - 1) / (2^M -1) instead of just 2^(N-M). Unless, of course, we are doing something to compensate for this when we fill out the A16 image data.

It's encoded on 16 bits. For an image with N bit depth (where N is 10 or 12). We need to multiply by:
2^(16-N), for 10 bits: 2^6=64, 12: 2^4=16

So yes, they are proper constants, and won't change so long that ColorDepth.bytes_per_pixel=2

Copy link
Contributor Author

jyavenard left a comment

They rely on one another, the need to both go at the same time.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kvark)

@jyavenard jyavenard force-pushed the jyavenard:1493198 branch from a80e926 to 1190782 Sep 24, 2018
@kvark
kvark approved these changes Sep 24, 2018
Copy link
Member

kvark left a comment

:lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


webrender_api/src/image.rs, line 142 at r1 (raw file):
ok, the comment was misleading me then:

Return how many bytes is used to encode a pixel's channel

We define the ColorDepth type in image as ultimately all types of images should support native color bit depth higher than 8 bits.

This change requires other patches found in bug 1493198
@jyavenard jyavenard force-pushed the jyavenard:1493198 branch from 1190782 to a9e5f4e Sep 24, 2018
@@ -359,6 +373,8 @@ impl TextureCache {

self.array_a8_linear
.update_profile(&mut texture_cache_profile.pages_a8_linear);
self.array_a16_linear

This comment has been minimized.

@gw3583

gw3583 Sep 24, 2018

Collaborator

If we only support R16 images via external image handles, then we can actually remove all the array_a16 code completely, and just add the new image format to the unreachable blocks in the texture cache. Do we ever want to support R16 images that are provided by CPU-side buffers? And if we do, perhaps we just want them to be standalone textures, rather than trying to fit them into a texture cache page?

@gw3583
Copy link
Collaborator

gw3583 commented Sep 25, 2018

@jyavenard Added a couple of questions about the texture cache code above. It's not a blocker - these only allocate texture cache pages lazily - but we could probably remove them if we don't think we want to support that code path. Thoughts?

@gw3583
Copy link
Collaborator

gw3583 commented Sep 25, 2018

From discussion on IRC, we decided to keep the texture cache support for now. It should be zero-cost if unused and may end up being used in the future for HDR image support etc.

@gw3583
Copy link
Collaborator

gw3583 commented Sep 25, 2018

@bors-servo r=kvark,gw3583

@bors-servo
Copy link
Contributor

bors-servo commented Sep 25, 2018

📌 Commit a9e5f4e has been approved by kvark,gw3583

Copy link
Contributor Author

jyavenard left a comment

Reviewable status: 11 of 13 files reviewed, 1 unresolved discussion (waiting on @gw3583)


webrender/src/texture_cache.rs, line 376 at r3 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

If we only support R16 images via external image handles, then we can actually remove all the array_a16 code completely, and just add the new image format to the unreachable blocks in the texture cache. Do we ever want to support R16 images that are provided by CPU-side buffers? And if we do, perhaps we just want them to be standalone textures, rather than trying to fit them into a texture cache page?

Done.

@bors-servo
Copy link
Contributor

bors-servo commented Sep 25, 2018

Testing commit a9e5f4e with merge 9a17291...

bors-servo added a commit that referenced this pull request Sep 25, 2018
Add support for 10/12 bits YUV images

We define the ColorDepth type in image as ultimately all types of images should support native color bit depth higher than 8 bits.

This change requires other patches found in bug 1493198

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3113)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 25, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark,gw3583
Pushing 9a17291 to master...

@bors-servo bors-servo merged commit a9e5f4e into servo:master Sep 25, 2018
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 2 files, 1 discussion left (gw3583)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@gw3583 gw3583 mentioned this pull request Sep 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.