Closed Bug 959154 Opened 6 years ago Closed 6 years ago

Moz2Dify GLUploadHelpers

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

Attachments

(8 files, 3 obsolete files)

1.39 KB, patch
nical
: review+
Details | Diff | Splinter Review
4.38 KB, patch
nical
: review+
Details | Diff | Splinter Review
8.43 KB, patch
nical
: review+
Details | Diff | Splinter Review
10.00 KB, patch
nical
: review+
Details | Diff | Splinter Review
10.41 KB, patch
nical
: review+
Details | Diff | Splinter Review
7.68 KB, patch
nical
: review+
Details | Diff | Splinter Review
26.55 KB, patch
Details | Diff | Splinter Review
8.55 KB, patch
Details | Diff | Splinter Review
Used to be a part of GLContext, but bug 877115 is getting bloated by comments and patches so let's break distinguishable parts out of it.
Blocks: 877115
Attachment #8375569 - Attachment is obsolete: true
Attachment #8375568 - Flags: review?(nical.bugzilla)
Attachment #8375570 - Flags: review?(nical.bugzilla)
Attachment #8375572 - Flags: review?(nical.bugzilla)
Attachment #8375576 - Flags: review?(nical.bugzilla)
Attachment #8375578 - Flags: review?(nical.bugzilla)
Attachment #8375579 - Flags: review?(nical.bugzilla)
Comment on attachment 8375581 [details] [diff] [review]
Part 7. Moz2Dify GLTextureImage::BeginUpdate/EndUpdate.

I am not sure of the implications to removing the SetDeviceOffset calls when moving from cairo surfaces to draw targets. Please enlighten me.

It didn't seem to cause any new reftest failures on my local build so I went ahead. I'll launch a full test run on try soon as well.
Attachment #8375581 - Flags: review?(nical.bugzilla)
Comment on attachment 8375582 [details] [diff] [review]
Part 8. Moz2Dify GLUploadHelpers.

Ah, joy!
Attachment #8375582 - Flags: review?(nical.bugzilla)
Attachment #8375568 - Flags: review?(nical.bugzilla) → review+
Attachment #8375570 - Flags: review?(nical.bugzilla) → review+
Attachment #8375572 - Flags: review?(nical.bugzilla) → review+
Attachment #8375576 - Flags: review?(nical.bugzilla) → review+
Attachment #8375578 - Flags: review?(nical.bugzilla) → review+
Attachment #8375579 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8375581 [details] [diff] [review]
Part 7. Moz2Dify GLTextureImage::BeginUpdate/EndUpdate.

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

::: gfx/gl/GLTextureImage.cpp
@@ +147,3 @@
>  
> +    if (!mUpdateDrawTarget) {
> +        mUpdateDrawTarget = nullptr;

nit: if mUpdateDrawTarget is null you don't really need to nullify it even more :)
Attachment #8375581 - Flags: review?(nical.bugzilla) → review+
Attachment #8375581 - Attachment is obsolete: true
Comment on attachment 8375646 [details] [diff] [review]
Part 7. Moz2Dify GLTextureImage::BeginUpdate/EndUpdate. (carries r=nical)

Hmm, I agree ;)
Attachment #8375646 - Attachment description: Part 7. Moz2Dify GLTextureImage::BeginUpdate/EndUpdate. → Part 7. Moz2Dify GLTextureImage::BeginUpdate/EndUpdate. (carries r=nical)
Attachment #8375582 - Flags: review?(nical.bugzilla) → review+
Some reftest failures indeed. I seem to have mixed up blue with red somehow in part 1.

I can reproduce the failures locally, although mach claims all these tests were good I can see them being wrong as they flicker by. Weird, but good enough for me.
Comment on attachment 8375568 [details] [diff] [review]
Part 1. Moz2Dify gl::UploadImageData.

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

::: gfx/gl/GLUploadHelpers.cpp
@@ +422,3 @@
>              if (gl->GetPreferredARGB32Format() == LOCAL_GL_BGRA) {
>                format = LOCAL_GL_BGRA;
> +              surfaceFormat = SurfaceFormat::B8G8R8A8;

to this. Well. Looks suspicious :)

@@ -421,3 @@
>              if (gl->GetPreferredARGB32Format() == LOCAL_GL_BGRA) {
>                format = LOCAL_GL_BGRA;
> -              surfaceFormat = gfx::SurfaceFormat::R8G8B8A8;

From this,
Attachment #8375568 - Attachment is obsolete: true
Attachment #8376185 - Attachment description: Part 1. Moz2Dify gl::UploadImageData. → Part 1. Moz2Dify gl::UploadImageData. (carries r=nical)
Looks good now. I'll wait for 10.8 to finish the tests before checking in.
The 10.8 reftest run doesn't even seem to start. Since they were exactly the same as the 10.6 failures before, I'll go ahead with checkin.
Keywords: checkin-needed
I believe I am innocent this time :)

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=314f24c02ea5 includes my patches and those oranges does not exist. They seem to be from the run before the backout (also caused by it) in that revision.

Ryan (or other sheriff), I'm setting checkin-needed again and you can double check.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.