Closed Bug 1382104 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Canvas: WebGL, enhancement, P1)

enhancement

Tracking

()

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

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

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

Attachments

(8 files)

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: → 1383907
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1322746
You need to log in before you can comment on or make changes to this bug.