On Wed, 17 Mar 2021 20:47:09 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> This is a fix for a long-standing bug where the D3D pipeline will stop
>> rendering when a Windows remote desktop session is disconnected and then
>> reconnected.
>>
>> A preliminary Draft PR #315 by @Schmidor was a good first step in solving
>> this. I took that and continued the work in my Draft PR #403. It is now
>> ready for formal review in this new PR. You can see PR #403 for details on
>> the history of the changes.
>>
>> ## Evaluation
>>
>> The root cause of this bug is that the D3D pipeline did not handle a return
>> code of `D3DERR_DEVICEREMOVED` from `TestCooperativeLevel`. When that error
>> occurs, an application needs to destroy and recreate the Direct3D device.
>>
>> The solution is to implement a new `D3DPipeline::reinitialize` method that
>> will destroy the native D3D device and dispose the existing ResourceFactory
>> objects and their associated BaseContext objects upon receiving
>> `D3DERR_DEVICEREMOVED`. Note that the `D3DPipeline` Java object singleton is
>> not recreated (it remains a singleton). In support of this, I implemented
>> proper disposal logic in `BaseResourceFactory` and `BaseContext` to clean
>> everything up and also to avoid memory leaks.
>>
>> Additionally, there were several places that assumed that some textures (and
>> mesh vertices) could be made permanent and never need to handle the case of
>> a lost device. These all had to be fixed to allow for the possibility of a
>> lost device and associated resource factory. They included:
>>
>> * UploadingPainter and PresentingPainter need to set the resource factory to
>> null when not ready, so it will get the (possibly new) factory the next time
>> it tries.
>> * The gradient texture cache in `PaintHelper` has to be cleared and
>> recreated when the surface is lost
>> * The 3D triangle mesh and Phong material classes need to be disposed when
>> the resource factory is disposed.
>> * WebView often renders to a texture image at a time other than from the
>> main rendering job, so needs to directly handle the case of a resource
>> factory that is lost.
>> * Decora PPSRenderer assumed that the resource factory never went away; it
>> also accessed it on the wrong thread. Both problems were addressed by
>> deferring the initialization of the resource factory and handling the case
>> where the device is disposed.
>> * Snapshot needs to allow for the platform image to be null if the device
>> has been disposed.
>>
>> ## Notes to Reviewers
>>
>> I created this PR from a branch that contains the original 4 commits by
>> @Schmidor (rebased on top of the current `master`) and then a single commit
>> on top of that to complete it. This allows anyone who is interested to
>> easily see the diffs between this PR and Oliver's original Draft PR. Most
>> reviewers can just go to the list of "Files" and see the aggregate diffs.
>>
>> During the course of my testing I discovered three outstanding problems,
>> which will be handled by filing follow-up issues. Once I file them, I'll add
>> a comment to this PR with the bug IDs.
>>
>> 1. Media: a media stream playing at the time of a reconnect doesn't continue
>> playing. Reloading the media works fine. This is not directly related to
>> this bug, since it also happens with the software pipeline.
>> 2. Canvas: doesn't preserve the contents after a device reconnect (noticed
>> while running Zoomy, where the BG color is wrong after device
>> reinitialization). This might point to a need to let the app know they have
>> to repaint, since there is no possible way to preserve the contents of the
>> texture when the device is lost.
>> 3. WebView: there is a possible memory leak when device isn't ready after
>> first reset, due to a `WCRenderQueueImpl::gc` instance being held in a
>> JNIGlobal. This looks like a preexisting condition that could happen with a
>> page (re)load today. It happens rarely.
>>
>> This is a complicated enough change that I'd like three reviewers. The bulk
>> of the changes are Windows-specific, but there are changes in common code so
>> at least a sanity check needs to be done on all platforms using both the HW
>> and SW pipelines. The case of a disposed device can currently only happen on
>> Windows with the D3D pipeline.
>
> Kevin Rushforth has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains six additional
> commits since the last revision:
>
> - Merge branch 'master' into 8239589-rdp-reconnect
> - 8239589: JavaFX UI will not repaint after reconnecting via Remote Desktop
> - Ressource already freed from pool
> - Recreate if adapterCount is zero
> - Fix whitespace error
> - 8239589: JavaFX UI will not repaint after reconnecting via Remote Desktop
I did a pass over the review and left a few inline comments pointing out minor
changes that I plan to make (in print statements and comments).
modules/javafx.graphics/src/main/java/com/sun/prism/d3d/D3DContext.java line
158:
> 156: int hr = D3DResourceFactory.nTestCooperativeLevel(pContext);
> 157:
> 158: if (PrismSettings.verbose && hr != D3D_OK) {
This makes `prism.verbose` too noisy, since it will sometimes continually print
non-failing return codes such as `S_PRESENT_MODE_CHANGED` or
`S_PRESENT_OCCLUDED`. Separately, I might file an RFE to handle
`S_PRESENT_MODE_CHANGED` which can happen in many cases (not related to this
bug) such as changing the resolution of of the screen or plugging / unplugging
a monitor.
I'll change this to: `PrismSettings.verbose && FAILED(hr)`
modules/javafx.graphics/src/main/java/com/sun/prism/d3d/D3DPipeline.java line
186:
> 184: void reinitialize() {
> 185: if (PrismSettings.verbose) {
> 186: System.err.println("D3DPipeline: reinitialize after device
> lost");
This probably should say "...device removed"
modules/javafx.graphics/src/main/java/com/sun/prism/impl/ps/PaintHelper.java
line 161:
> 159:
> 160: // gradientCacheTexture is left permanent and locked, although
> we still
> 161: // must check for isSurfaceLost() is the case the device is
> disposed.
Typo: "is the case..." --> "in case..."
modules/javafx.graphics/src/main/java/com/sun/prism/impl/ps/PaintHelper.java
line 174:
> 172:
> 173: // gtexCacheTexture is left permanent and locked, although we
> still
> 174: // must check for isSurfaceLost() is the case the device is
> disposed.
Typo: "is the case..." --> "in case..."
-------------
PR: https://git.openjdk.java.net/jfx/pull/430
