Closed
Bug 721467
Opened 12 years ago
Closed 12 years ago
Add a codepath to only use glTexImage2D instead of glTexSubImage2D when texture uploading in GLContext
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: gw280, Assigned: gw280)
References
Details
Attachments
(1 file, 3 obsolete files)
6.79 KB,
patch
|
Details | Diff | Splinter Review |
Some drivers don't do glTexSubImage2D very well. We should have a code path that avoids using that function in GLContext
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #591894 -
Flags: review?(bgirard)
Assignee | ||
Updated•12 years ago
|
Attachment #591894 -
Flags: review?(bgirard) → review?(joe)
Assignee | ||
Updated•12 years ago
|
Attachment #591894 -
Flags: review?(bgirard)
Comment 2•12 years ago
|
||
Comment on attachment 591894 [details] [diff] [review] use glTexImage2D Review of attachment 591894 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Make a couple of changes and you should be good to go! ::: gfx/gl/GLContext.cpp @@ +2076,5 @@ > + if (iterRect->x == 0 && > + iterRect->y == 0 && > + iterRect->width == aTextureSize.width && > + iterRect->height == aTextureSize.height) > + useTexSubImage2D = false; Add {} ::: gfx/gl/GLContext.h @@ +1214,5 @@ > GLuint& aTexture, > bool aOverwrite = false, > const nsIntPoint& aSrcPoint = nsIntPoint(0, 0), > + bool aPixelBuffer = false, > + const nsIntSize& aTextureSize = nsIntSize(0, 0)); I don't think it makes sense for aTextureSize to have a default value. It should just be added to the earlier, non-default parameters. Also, document the parameter.
Attachment #591894 -
Flags: review?(joe) → review+
Comment 3•12 years ago
|
||
Comment on attachment 591894 [details] [diff] [review] use glTexImage2D Review of attachment 591894 [details] [diff] [review]: ----------------------------------------------------------------- Excellent patch! ::: gfx/gl/GLContext.cpp @@ +400,5 @@ > } > } > + > + glRendererString = (const char *)fGetString(LOCAL_GL_RENDERER); > + const char *rendererMatchStrings[RendererOther] = { The name implies string equality. It should be renamed to something like 'DoesSubStringMatch' @@ +2075,5 @@ > + // the entire texture > + if (iterRect->x == 0 && > + iterRect->y == 0 && > + iterRect->width == aTextureSize.width && > + iterRect->height == aTextureSize.height) I think we should do >=. I know with the call site this wont happen but let's make this function more generic and let it grows the texture if it needs to.
Attachment #591894 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #591894 -
Attachment is obsolete: true
Attachment #591921 -
Flags: review?(joe)
Comment 5•12 years ago
|
||
Comment on attachment 591921 [details] [diff] [review] updated patch Review of attachment 591921 [details] [diff] [review]: ----------------------------------------------------------------- I checked the UploadSurfaceToTexture call sites, and the size arguments all look correct.
Attachment #591921 -
Flags: review?(joe) → review+
Comment 6•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9699edcbcedd
Assignee: nobody → gwright
Target Milestone: --- → mozilla12
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9699edcbcedd
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 8•12 years ago
|
||
Backed out for causing bug 722167: https://hg.mozilla.org/mozilla-central/rev/d10da858631a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 9•12 years ago
|
||
This removes the general optimisation that was causing bug 722167
Attachment #591921 -
Attachment is obsolete: true
Attachment #592822 -
Flags: review?(joe)
Assignee | ||
Comment 10•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=d338c6ebc498 New patched has been pushed to try. I've run a local build on my OS X box and confirmed it no longer causes gfx corruption.
Comment 11•12 years ago
|
||
Comment on attachment 592822 [details] [diff] [review] updated patch removing the glTexImage2D general case optimisation Review of attachment 592822 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContext.cpp @@ +712,5 @@ > // determine the region the client will need to repaint > + if (!mGLContext->CanUploadSubTextures()) { > + aRegion = nsIntRect(nsIntPoint(0, 0), mSize); > + } else { > + GetUpdateRegion(aRegion); Reverse these two so we don't have a negation @@ +2012,5 @@ > // to the start of the data block. > if (!aPixelBuffer) { > data = imageSurface->Data(); > } > + Spaces on this line. @@ +2079,5 @@ > "Must be uploading to the origin when we don't have an existing texture"); > > + bool useTexSubImage2D = true; > + > + nsIntRect bounds = aDstRegion.GetBounds(); You can remove these two lines.
Attachment #592822 -
Flags: review?(joe) → review+
Assignee | ||
Comment 12•12 years ago
|
||
Final patch for committing
Attachment #592822 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 13•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb06e8a06ff3
Keywords: checkin-needed
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/eb06e8a06ff3
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla12 → mozilla13
You need to log in
before you can comment on or make changes to this bug.
Description
•