Mir

Code review comment for lp:~vanvugt/mir/bypass

Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

> 1. Purifying: virtual void post_update(std::shared_ptr<Buffer> /* bypass_buf */) {}
> I think it would be worse to ask platforms which don't implement bypass to implement
> an unused stub. That's just going to confuse people and bloat the code with unused stub methods.

All of our interface classes are pure abstract, and it pays to be consistent. I don't think it's such a big overhead to add a stub method to a platform if it doesn't support bypass.

> 2. "Unused" last_flipped_bypass_buf is actually important and very much used.
> It holds a reference to the TemporaryBuffer being scanned out for the lifetime of the frame.
> Without it the buffer will get reused too soon and you will see that as tearing.

OK. Perhaps a comment here would help to make the intent more apparent.

> 3. Ugly casting... Your suggestion probably is not possible exactly how you put it.
> It's a TemporaryBuffer so the get_gbm_bo function would have to exist in the common
> interfaces for all platforms. I did have a similar generic solution implemented until
> r891, but decided using native_buffer_handle was more appropriate and avoided polluting
> non-GBM platforms. Please bzr diff -r891..890 and tell me if that would be preferable.

How about having a MirGBMBufferPackage struct which extends MirBufferPackage? We can then cast the returned MirNativeBuffer struct that GBMBuffer::native_buffer_handle() returns to MirGBMBufferPackage to gain access to GBM specific server-side data (like the gbm_bo).

235 +BypassFilter::BypassFilter(const graphics::DisplayBuffer &display_buffer)
...and at other points

'T const&' is the preferred style.

« Back to merge proposal