Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Persistent Metal swapchain #2169

Merged
merged 6 commits into from
Jun 23, 2018
Merged

Persistent Metal swapchain #2169

merged 6 commits into from
Jun 23, 2018

Conversation

kvark
Copy link
Member

@kvark kvark commented Jun 21, 2018 •
edited
Loading

Includes #2164
Fixes #2168
Fixes #2143
Removes deferred image resolution (yay!), and also renames some of the outdated concepts.
We have no flickering, and proper frame sync now.

PR checklist:

  • make succeeds (on *nix)
  • make reftests succeeds
  • tested examples with the following backends:

@kvark kvark force-pushed the mtl-swapchain branch 2 times, most recently from 8de0506 to 5482b73 Compare June 22, 2018 15:35
@kvark
Copy link
Member Author

kvark commented Jun 22, 2018

I've been looking at the resize issue with this approach, and haven't got a working solution yet. It appears that CAMetalLayer does a cleanup routine on each nextDrawable call, which involves freeing any MTLIGAccelTexture and IOSurface that are not needed:

   0 libobjc.A.dylib -[NSObject release]
   1 QuartzCore release_image(_CAImageQueue*, unsigned long, CAImageQueueReleased*, bool)
   2 QuartzCore CAImageQueueCollect_
   3 QuartzCore -[CAMetalLayer nextDrawable]
   4 quad _$LT$$LP$$RP$$u20$as$u20$objc..message..MessageArguments$GT$::invoke::h1b3182d6d03268e8 /Users/dmalyshau/.cargo/registry/src/github.com-1ecc6299db9ec823/objc-0.2.2/src/message/mod.rs:123
   5 quad objc::message::send_unverified::h3d8bba873fb684c6 /Users/dmalyshau/.cargo/registry/src/github.com-1ecc6299db9ec823/objc-0.2.2/src/message/mod.rs:187
   6 quad objc::message::send_message::h70a5434b096fefd8 src/backend/metal/src/window.rs:197
   7 quad gfx_backend_metal::window::SurfaceInner::next_frame::h03e08cb6c11f5ce5 src/backend/metal/src/window.rs:48
   8 quad gfx_backend_metal::window::SwapchainImage::wait_until_ready::h7046cc58479c4f64 src/backend/metal/src/window.rs:170
   9 quad gfx_backend_metal::command::CommandQueue::wait::h3fa7880b032ce033 /Users/dmalyshau/Code/gfx/src/backend/metal/src/command.rs:1212
  10 quad _$LT$gfx_backend_metal..command..CommandQueue$u20$as$u20$gfx_hal..queue..RawCommandQueue$LT$gfx_backend_metal..Backend$GT$$GT$::submit_raw::h5e304cc3bfc623b7 /Users/dmalyshau/Code/gfx/src/backend/metal/src/command.rs:1228
  11 quad _$LT$gfx_hal..queue..CommandQueue$LT$B$C$$u20$C$GT$$GT$::submit::h8b44ace069ee5932 /Users/dmalyshau/Code/gfx/src/hal/src/queue/mod.rs:115
  12 quad quad::main::hd8ef08905678179a examples/quad/main.rs:504
  13 quad std::rt::lang_start::_$u7b$$u7b$closure$u7d$$u7d$::he6e7766661d087f6 /Users/travis/build/rust-lang/rust/src/libstd/rt.rs:74
  14 quad std::rt::lang_start_internal::_$u7b$$u7b$closure$u7d$$u7d$::h119e0490912ce91a libstd/panicking.rs:59
  15 quad std::panicking::try::do_call::hd1b51a136314709e libstd/panicking.rs:306
  16 quad __rust_maybe_catch_panic libpanic_unwind/lib.rs:102
  17 quad std::rt::lang_start_internal::hb7855285a8133f7a libstd/rt.rs:285
  18 quad std::rt::lang_start::h2db61e66a5bb226a /Users/travis/build/rust-lang/rust/src/libstd/rt.rs:74

... except that it decides to not clean up those images we use persistently. I did confirm that the refcount on them (meaning MTLIGAccelTexture) is 1 by the point of swapchain re-creation, and it matches the old code behavior (which works). So the problem lies something else... something prevents CAMetalLayer to not free those surfaces.

@kvark kvark force-pushed the mtl-swapchain branch 2 times, most recently from a132708 to 0d8c23f Compare June 23, 2018 03:34
grovesNL
grovesNL reviewed Jun 23, 2018
View reviewed changes
src/backend/metal/src/window.rs Outdated
(drawable, msg_send![drawable, texture])
};

//debug!("looking for {:?}", texture_temp);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: comment

src/backend/metal/src/window.rs
}
}
// wait for new frames to come until we meet the chosen one
while self.surface.next_frame(&self.frames).0 != self.index as usize {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a low-cost alternative to spinning here? I don't know how long these waits commonly last, but it seems like there's potential to run faster than presents occur sometimes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could do a better job at locking here, even though locking a mutex would only hurt on contention (and these frame locks are unlikely to content).

there's potential to run faster than presents occur sometimes

The method is only called at submission time, this is as late as we could possibly be for obtaining CAMetalDrawable. I don't think we can do better atm.

src/backend/metal/src/window.rs Outdated
//TODO: only set it where supported
msg_send![render_layer, setDisplaySyncEnabled: display_sync];
//msg_send![render_layer, setPresentsWithTransaction: true];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: comment

src/backend/metal/src/window.rs Outdated
// when resizing, this trick frees up the currently shown frame
drawable.present();
}
//info!("\tframe[{}] = {:?}", index, texture);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: comment

@kvark
Copy link
Member Author

kvark commented Jun 23, 2018

Thanks for the review! Comments are addressed now, let's ship this hack :)
bors r=grovesNL

bors bot added a commit that referenced this pull request Jun 23, 2018
@bors @kvark
Merge #2169
f2f7c5d 
2169: Persistent Metal swapchain r=grovesNL a=kvark

Includes #2164
Fixes #2168
Fixes #2143
Removes deferred image resolution (yay!), and also renames some of the outdated concepts.
We have no flickering, and proper frame sync now.

PR checklist:
- [x] `make` succeeds (on *nix)
- [x] `make reftests` succeeds
- [x] tested examples with the following backends:


Co-authored-by: Dzmitry Malyshau <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jun 23, 2018

Build succeeded

@bors bors bot merged commit 1f4cbc4 into gfx-rs:master Jun 23, 2018
@kvark kvark deleted the mtl-swapchain branch June 24, 2018 02:10
@kvark kvark mentioned this pull request Jun 26, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grovesNL grovesNL grovesNL left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kvark @grovesNL