Closed Bug 1382104 Opened 8 years ago Closed 7 years ago

Internal-vs-user bound FB divide is more trouble than it's worth

Categories

(Core :: Graphics: CanvasWebGL, enhancement, P1)

Product:
Core
Component:
Graphics: CanvasWebGL
Type:
enhancement

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1322746
Tracking Flags:
Tracking Status
firefox57 --- wontfix

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

(Keywords: stale-bug, Whiteboard: gfx-noted)

Attachments

(8 files)

Bug 1382104 - Remove internal-vs-user FB bindings. -
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
Bug 1382104 - Set viewport and colormask for draw-blit. -
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
Bug 1382104 - Update CompositorOGL handling of offscreen GLContext default framebuffers. -
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1382104 - Fix dom/vr reftests. -
59 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1382104 - Warn in VRDIsplay.isPresenting if the display has been forcibly shutdown. -
59 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1382104 - Remove IsMirror concept in favor of checking forwarders. -
59 bytes, text/x-review-board-request
kip
: review+
Details
Bug 1382104 - Don't call GL commands if MakeCurrent failed. -
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
Bug 1382104 - gfxVRPuppet should not expose padding as data. -
59 bytes, text/x-review-board-request
kip
: review+
Details
No description provided.
Whiteboard: gfx-noted
Blocks: 1322746
Comment on attachment 8887776 [details] Bug 1382104 - Remove internal-vs-user FB bindings. - https://reviewboard.mozilla.org/r/158702/#review163952 LGTM. please check if you wanna remove the functions that you marked. ::: gfx/gl/GLContext.h:3342 (Diff revision 1) > * Return size of this offscreen context. > * > * Only valid if IsOffscreen() returns true. > */ > const gfx::IntSize& OffscreenSize() const; > - > +/* Are you planning to remove them or wanna remain and mark them?
Attachment #8887776 - Flags: review?(dmu) → review+
Will remove.
Comment on attachment 8887776 [details] Bug 1382104 - Remove internal-vs-user FB bindings. - https://reviewboard.mozilla.org/r/158702/#review163952 > Are you planning to remove them or wanna remain and mark them? Will remove them.
Pushed by jgilbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6e571ab7b558 Remove internal-vs-user FB bindings. - r=daoshengmu
Backed out for asserting at dom/canvas/WebGLContextUtils.cpp:714 in browser-chrome tests and failing GTest's Gfx.CompositorSimpleTree: https://hg.mozilla.org/integration/mozilla-inbound/rev/aef0921e8682aef490ffb00fab256f0b4b0a0954 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6e571ab7b558eeee419a8c9032f756f0c44bb554&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable Failure log assertion: https://treeherder.mozilla.org/logviewer.html#?job_id=115529498&repo=mozilla-inbound 23:47:09 INFO - TEST-START | toolkit/modules/tests/browser/browser_Troubleshoot.js 23:47:09 INFO - GECKO(1939) | Chrome file doesn't exist: /Users/cltbld/tasks/task_1500445269/build/tests/mochitest/browser/toolkit/modules/tests/browser/head.js 23:47:09 INFO - GECKO(1939) | Failed 0x8ca6 shadow: Cached 0x6/6, should be 0x5/5. 23:47:09 INFO - GECKO(1939) | Assertion failure: false (Bad cached value.), at /home/worker/workspace/build/src/dom/canvas/WebGLContextUtils.cpp:714 23:47:09 INFO - GECKO(1939) | #01: mozilla::WebGLContext::SetDimensions(int, int) [dom/canvas/WebGLContext.cpp:1144] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #02: mozilla::dom::CanvasRenderingContextHelper::UpdateContext(JSContext*, JS::Handle<JS::Value>, mozilla::ErrorResult&) [dom/canvas/CanvasRenderingContextHelper.cpp:249] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #03: mozilla::dom::CanvasRenderingContextHelper::GetContext(JSContext*, nsAString const&, JS::Handle<JS::Value>, mozilla::ErrorResult&) [dom/canvas/CanvasRenderingContextHelper.cpp:204] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #04: mozilla::dom::HTMLCanvasElement::GetContext(JSContext*, nsAString const&, JS::Handle<JS::Value>, mozilla::ErrorResult&) [dom/html/HTMLCanvasElement.cpp:987] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #05: mozilla::dom::HTMLCanvasElementBinding::getContext(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLCanvasElement*, JSJitMethodCallArgs const&) [mfbt/AlreadyAddRefed.h:143] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #06: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) [dom/bindings/BindingUtils.cpp:3062] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #07: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [js/src/jscntxtinlines.h:294] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #08: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:470] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #09: <name omitted> [js/src/vm/Interpreter.cpp:534] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #10: js::Wrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const [js/src/proxy/Wrapper.cpp:169] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #11: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const [js/src/proxy/CrossCompartmentWrapper.cpp:359] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #12: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) [js/src/proxy/Proxy.cpp:481] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #13: js::proxy_Call(JSContext*, unsigned int, JS::Value*) [js/public/RootingAPI.h:825] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #14: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [js/src/jscntxtinlines.h:294] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #15: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:452] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #16: Interpret(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:3066] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #17: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:410] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #18: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:488] 23:47:09 INFO - 23:47:09 INFO - GECKO(1939) | #19: js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) [js/src/jit/BaselineIC.cpp:2554] 23:47:09 INFO - Failure GTest: https://treeherder.mozilla.org/logviewer.html#?job_id=115529515&repo=mozilla-inbound 23:39:56 INFO - TEST-START | Gfx.CompositorSimpleTree 23:39:56 INFO - [1622] WARNING: Framebuffer not complete -- CheckFramebufferStatus returned 0x8219, GLContext=15850f000, IsOffscreen()=1, mFBO=0, aFBOTextureTarget=0x0, aRect.width=256, aRect.height=256: file /home/worker/workspace/build/src/gfx/layers/opengl/CompositingRenderTargetOGL.cpp, line 65 23:39:56 INFO - Layer Tree: 23:39:56 INFO - LayerManager (0x100579440) --- in content order 23:39:56 INFO - ContainerLayerComposite (0x1788a8c00) [shadow-visible=< (x=0, y=0, w=256, h=256); >] [visible=< (x=0, y=0, w=256, h=256); >] { hitregion=< (x=0, y=0, w=256, h=256); >} 23:39:56 INFO - ColorLayerComposite (0x1788a9000) [shadow-visible=< (x=0, y=0, w=256, h=256); >] [visible=< (x=0, y=0, w=256, h=256); >] { hitregion=< (x=0, y=0, w=256, h=256); >} [color=rgba(255, 0, 255, 1.000000)] [bounds=(x=0, y=0, w=256, h=256)] 23:39:56 INFO - ColorLayerComposite (0x1788a9400) [shadow-visible=< (x=0, y=0, w=100, h=100); >] [visible=< (x=0, y=0, w=100, h=100); >] { hitregion=< (x=0, y=0, w=100, h=100); >} [color=rgba(255, 0, 0, 1.000000)] [bounds=(x=0, y=0, w=100, h=100)] 23:39:56 INFO - ColorLayerComposite (0x1788a9800) [shadow-visible=< (x=0, y=50, w=100, h=100); >] [visible=< (x=0, y=50, w=100, h=100); >] { hitregion=< (x=0, y=50, w=100, h=100); >} [color=rgba(0, 0, 255, 1.000000)] [bounds=(x=0, y=50, w=100, h=100)] 23:39:56 INFO - Original: ... 23:39:56 INFO - Reference: ... 23:39:56 WARNING - TEST-UNEXPECTED-FAIL | Gfx.CompositorSimpleTree | Value of: CompositeAndCompare(layerManager, refDT) 23:39:56 INFO - Actual: false 23:39:56 INFO - Expected: true @ /home/worker/workspace/build/src/gfx/tests/gtest/TestCompositor.cpp:206
Flags: needinfo?(jgilbert)
(In reply to Jeff Gilbert [:jgilbert] from comment #2) > Looking green for m-gl: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=a443a6f77764a74ed7e7875a933c00689785156c Looks like OSX never got around to running tests on try, even 7+ hours post-push. It failed on inbound, but I can't exactly feel bad about it.
This also had a ton of Windows reftest failures.
Comment on attachment 8897224 [details] Bug 1382104 - Set viewport and colormask for draw-blit. - https://reviewboard.mozilla.org/r/168498/#review173836 LGTM
Attachment #8897224 - Flags: review?(dmu) → review+
Comment on attachment 8897229 [details] Bug 1382104 - Don't call GL commands if MakeCurrent failed. - https://reviewboard.mozilla.org/r/168508/#review174320 r=me
Attachment #8897229 - Flags: review?(dmu) → review+
Comment on attachment 8897226 [details] Bug 1382104 - Fix dom/vr reftests. - https://reviewboard.mozilla.org/r/168502/#review174708 lgtm, thanks!
Attachment #8897226 - Flags: review?(kgilbert) → review+
Comment on attachment 8897227 [details] Bug 1382104 - Warn in VRDIsplay.isPresenting if the display has been forcibly shutdown. - https://reviewboard.mozilla.org/r/168504/#review174710 minor nit on potentially redundant do/while(false) loop. This is okay, but wish to make sure this pattern was intentional (avoiding goto's?). r=me either way if simplified with /s/break/return false/ or just a clever new pattern ;-) ::: dom/vr/VRDisplay.cpp:688 (Diff revision 1) > - return mPresentation != nullptr; > + if (mPresentation) > + return true; > + > + do { > + if (!mShutdown) > + break; is there any reason not to just "return false" instead of "break" here and avoid the do/while loop?
Attachment #8897227 - Flags: review?(kgilbert) → review+
Comment on attachment 8897227 [details] Bug 1382104 - Warn in VRDIsplay.isPresenting if the display has been forcibly shutdown. - https://reviewboard.mozilla.org/r/168504/#review174710 > is there any reason not to just "return false" instead of "break" here and avoid the do/while loop? This looks like a relic of something else that was going on, but has since been removed. These should just be returns.
Comment on attachment 8897225 [details] Bug 1382104 - Update CompositorOGL handling of offscreen GLContext default framebuffers. - https://reviewboard.mozilla.org/r/168500/#review175672
Attachment #8897225 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8897228 [details] Bug 1382104 - Remove IsMirror concept in favor of checking forwarders. - https://reviewboard.mozilla.org/r/168506/#review176034 This LGTM, thanks Jeff! When you have a chance, I would like to share with you a WIP patch I have for Bug 1381085. This also eliminates the need for the mirror functionality and enables us to remove most VR specialized paths in ImageChild/Parent/Host/Client. I think you could land this patch first, but then could be simplified even further with the patch in Bug 1381085.
Attachment #8897228 - Flags: review?(kgilbert) → review+
Comment on attachment 8899028 [details] Bug 1382104 - gfxVRPuppet should not expose padding as data. - https://reviewboard.mozilla.org/r/170344/#review176036 LGTM, thanks!
Attachment #8899028 - Flags: review?(kgilbert) → review+
No longer blocks: 1322746
See Also: → 1322746
See Also: → 1381085
See Also: → 1383907
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: