Closed
Bug 1081363
Opened 10 years ago
Closed 10 years ago
Don't do readback internal to ShSurf_Basic
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: jgilbert, Assigned: jgilbert)
References
Details
Attachments
(1 file)
14.03 KB,
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
Let's consider ShSurf_Basic as not having any destination form, thus requiring a readback, rather than having it be yet another readback path. We already have readback paths for cases where we can't handle the type of the ShSurf. We should just have that code handle Basic surfaces.
Assignee | ||
Comment 1•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e1c300caf123
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8503454 -
Flags: review?(dglastonbury)
Assignee | ||
Comment 3•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e9e9426d2bca
Depends on: 1066280
Comment on attachment 8503454 [details] [diff] [review] 0001-Don-t-do-readback-eagerly-in-ShSurf_Basic.patch Review of attachment 8503454 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/SharedSurface.cpp @@ +491,5 @@ > + if (!dst->LockBits(&dstBytes, &dstSize, &dstStride, &dstFormat)) > + return false; > + > + const bool isDstRGBA = dstFormat == gfx::SurfaceFormat::R8G8B8A8 || > + dstFormat == gfx::SurfaceFormat::R8G8B8X8; I wouldn't mind some parens to separate the == || == part from the assignment.
Attachment #8503454 -
Flags: review?(dglastonbury) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Dan Glastonbury :djg :kamidphish from comment #4) > Comment on attachment 8503454 [details] [diff] [review] > 0001-Don-t-do-readback-eagerly-in-ShSurf_Basic.patch > > Review of attachment 8503454 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/gl/SharedSurface.cpp > @@ +491,5 @@ > > + if (!dst->LockBits(&dstBytes, &dstSize, &dstStride, &dstFormat)) > > + return false; > > + > > + const bool isDstRGBA = dstFormat == gfx::SurfaceFormat::R8G8B8A8 || > > + dstFormat == gfx::SurfaceFormat::R8G8B8X8; > > I wouldn't mind some parens to separate the == || == part from the > assignment. We generally don't do this elsewhere, but I wouldn't otherwise be adverse to it. It's a fairly standard pattern, so we can be confident the order-of-operations should be obvious.
(In reply to Jeff Gilbert [:jgilbert] from comment #5) > (In reply to Dan Glastonbury :djg :kamidphish from comment #4) > > Comment on attachment 8503454 [details] [diff] [review] > > 0001-Don-t-do-readback-eagerly-in-ShSurf_Basic.patch > > > > Review of attachment 8503454 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: gfx/gl/SharedSurface.cpp > > @@ +491,5 @@ > > > + if (!dst->LockBits(&dstBytes, &dstSize, &dstStride, &dstFormat)) > > > + return false; > > > + > > > + const bool isDstRGBA = dstFormat == gfx::SurfaceFormat::R8G8B8A8 || > > > + dstFormat == gfx::SurfaceFormat::R8G8B8X8; > > > > I wouldn't mind some parens to separate the == || == part from the > > assignment. > > We generally don't do this elsewhere, but I wouldn't otherwise be adverse to > it. > It's a fairly standard pattern, so we can be confident the > order-of-operations should be obvious. It just helps break up the a = b == c, which looks a bit odd to me. Also Emacs automatically gets the indentation correction when you put ()'s. I know it doesn't effect the way the code is compiled but helps my dinosaur brain parse what's on the screen. :-)
Assignee | ||
Comment 7•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=19a8c6e9f671
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0c17c73ba1d
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a0c17c73ba1d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•