-
Notifications
You must be signed in to change notification settings - Fork 219
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.
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
Qt preview #69
Qt preview #69
Conversation
@naushir This provides a preview window implemented with Qt. Why on earth would we want that, especially when it involves buffer copying, CPU resizing and colour conversions...? Well, it works with X forwarding, so it does provide an option for people who really feel they have to have a local preview window when the Pi running the camera is somewhere else. |
if (options->nopreview) | ||
return make_null_preview(options); | ||
else if (options->qt_preview) | ||
return make_qt_preview(options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need an #if QT_PRESET
type guard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
/* SPDX-License-Identifier: BSD-2-Clause */ | ||
/* | ||
* Copyright (C) 2020, Raspberry Pi (Trading) Ltd. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2021 :)
@@ -20,7 +20,7 @@ class DrmPreview : public Preview | |||
~DrmPreview(); | |||
// Display the buffer. You get given the fd back in the BufferDoneCallback | |||
// once its available for re-use. | |||
virtual void Show(int fd, size_t size, int width, int height, int stride) override; | |||
virtual void Show(int fd, void *mem, size_t size, int width, int height, int stride) override; | |||
// Reset the preview window, clearing the current buffers and being ready to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too fussed about this, but perhaps we should use a Span for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that makes sense. Our post-processing branch has switched over to using Spans more so this will fit well (though probably cause a minor merge conflict!).
// is only that there might be some tearing, so I don't think we worry. We could speed | ||
// it up by getting the ISP to supply RGB, but I'm not sure I want to handle that extra | ||
// possibility in our main application code, so we'll put up with the slow conversion. | ||
for (int y = 0; y < window_height_; y++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps also an assert to make sure we are coming into here with a YUV420 format image?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the preview windows only support YUV420 so we probably want to check back in the LibcameraApp::previewThread, but doing so would clearly be sensible.
This takes some slightly ugly decision code out of the LibcameraApp base class, leaving things tidier. In future it will be less painful if we add new implementations of the preview window. Signed-off-by: David Plowman <[email protected]>
This will allow for preview windows that want memory pointers rather than file descriptors. Signed-off-by: David Plowman <[email protected]>
We don't currently handle anything else. Signed-off-by: David Plowman <[email protected]>
The Qt preview window is computationally very expensive and should normally be avoided. It does buffer copying, resizing and colour space conversion all using the CPU. However, it does work with X forwarding so may have some application when a remote preview window is absolutely necessary. The Qt preview has to be requested explicitly via the "--qt-preview" option, we never make it automatically. Signed-off-by: David Plowman <[email protected]>
90a8245
to
9249334
Compare
@naushir I think this latest version takes care of those various things you pointed out - let me know what you think! |
Looks good! |
No description provided.