-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
CameraView: Allow to change the camera resolution and framerate #224
base: main
Are you sure you want to change the base?
Conversation
Add a popover to the camera view to change the resolution and framerate.
5f795d6
to
c711567
Compare
@tintou this seems functional to start. I think UI-wise it would make sense to move it to the existing Settings menu. I'm not sure what the least-bad way to present all those options (that most people shouldn't have to change!) is. My initial thought to not overwhelm the popover is a combobox but I don't love it. |
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.
Recent changes to master conflict with this PR as it stands. In particular, secondary clicking on the view now takes a photo - it does not trigger the resolution popover. So either that change must be reverted or the resolution menu be added to the AppMenu button (or its own button?) instead.
When I disabled taking a photo with a secondary click then the popover showed as expected. Clicking on most options changed the resolution of the view as expected but did not close the popover so the effect was partially obscured by the popover. I think it would be better to close the popover after resolution selection but that is a UX decision.
I noticed that some entries appeared duplicate (presumably some other capability was different) and at least one of the lower resolution options consistently caused the view to freeze after which changing the resolution back did not restart the pipeline.
For now I have put the resolution menu in a separate headerbar button as I am uncertain of the best way to include a fairly long list of resolutions in the AppMenu popover. I have removed most of the duplicates by showing only video capabilities but I notice from the screenshot that there is still one apparent duplicate. Investigating. |
Using
Should these be de-duplicated? |
The last commit makes the preview resolution independent of the photo capture resolution but follows the video capture resolution when in video mode. When in photo mode, the preview resolution framerate is kept to at least a minimum (currently set to 10fps). |
I think this is ready for functional review now. Final UX to be decided. Tbh I think the code could be improved by moving most control functions into MainWindow to make it easier to coordinate the view and the headerbar. But I do not want to increase the diff further in this PR. |
Having problems resolving conflicts with master ... |
# Conflicts: # src/MainWindow.vala
capsfilter.get_static_pad ("src").add_probe (Gst.PadProbeType.EVENT_BOTH, (pad, info) => { | ||
unowned Gst.Event? event = info.get_event (); | ||
if (event.type == Gst.EventType.CAPS) { | ||
//TODO Find correct resolution index and update action state. |
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.
I have inserted a placeholder here as the existing code did not seem to be working and I am not sure under what circumstances this event would get triggered other than through user interaction with the app (which is already catered for).
Just found a bug so converting back to draft. |
# Conflicts fixed in: # src/Widgets/CameraView.vala
This seems to work as expected, but I can't really confirm a noticeable improvement in performance here. I'm still getting stuttering even at low resolutions. Is that something you're experiencing @jeremypw ? |
I have regarded and tested this as mainly aiming at providing different resolutions of the saved photo or video. Maybe problems with stuttering can be dealt with separately? |
I can't say I I've noticed significant stuttering. Is this something this PR introduced? |
@danrabbit I can confirm that video stutters using master so this does not seem to be related to this PR (which does not claim to fix this issue). |
@jeremypw yeah as far I understand the goal of selecting a different camera resolution was to improve performance, especially with higher resolution cameras which were basically unusable. I'm not really sure of the value of reducing the resolution of the saved photo or video outside of that context? |
@danrabbit I was thinking that maybe e.g. for posting on the web a lower resolution/framerate might be desirable (not such a problem these days I guess). The poor performance seems to be related to something else since this PR does not fix it. |
@danrabbit Is this PR of sufficient value as is to be merged, or do you want it to fix the performance issue as well? @tintou Would welcome your input as I have amended your original PR somewhat. |
@jeremypw I'm not sure what the point of lowering the camera frame rate or resolution would be if it doesn't improve performance. I don't think anybody had requested that as a feature. My understanding is that this branch was an attempt at resolving performance issues, so since it doesn't do that it's probably not worth merging imo |
@danrabbit OK, I'll look into the performance issue more closely. I am a bit surprised that choosing a high frame-rate mode over a low frame-rate mode does not improve performance though. |
@tintou I do not really want to pursue this PR any further. Maybe better start afresh with a focus on performance improvement, which seems to be key and which this PR does not solve. I'll leave it to you to close it if you do not want to pursue either. |
Add a popover to the camera view to change the resolution and framerate.