Solve logical errors in configure_ and some more refactors of picamera2 module

[pantheraleo-7]

Follow-up of #1234

• 1st commit: minor refactors (partly based on #1234(comment), #1177(comment), #1248(comment))
• 2nd commit, resolved the following:
  1. Only CameraConfiguration configs were getting patched, not dict.
  2. L1089 sets the self.libcamera_config before validation.
  3. the else block would update the video_configuration property even when a dict or CameraConfiguration config was passed.

[pantheraleo-7]

When the user passes a string name or a CameraConfiguration object to configure(), the raw stream format and size gets patched at L1068 but when the user passes a dict we only copy it and doesn't convert it to a CameraConfirguration L1066, hence the patches don't apply. What is the rationale behind this?

[pantheraleo-7]

@davidplowman

[davidplowman]

Hi, and sorry for the delay. I've been a bit pre-occupied lately doing AI models. Who isn't, these days!!

I'm struggling a bit to remember all this configuration stuff, and there is quite a lot of history there. Here's what comes to mind:

• Originally there were only dict configurations, but then some people wanted configuration objects instead, and then they wanted not to have to create the objects and pass them around... so there's been quite a bit of churn.

• At one point we found libcamera works better if you always request a raw stream (even if you don't plan to use it). For dict configurations, this happens in the create_xxx_configuration functions.

• But for configuration objects, we have to do some patching up here, because there isn't another convenient place to change the raw stream parameters.

• The idea was always that your config would be updated to show you what you finally got (though I'm not sure if that works for configuration objects).

• I do wonder if the copy that happens to dict configuration is redundant, as the dict that is passed in seems to get updated anyway.

I definitely think there's some rationalisation that needs to happen, though I worry a bit that it would break something for someone. But we should probably take the hit at some point.

Sorry if that's not super helpful...

[pantheraleo-7]

camera_config can be of 3 types: str, dict or CameraConfiguration.

if the user passes in a str - it refers to the "standard" configurations by name, namely: preview, still and video

but if the user passes a dict or CameraConfiguration - now this is a "custom" configuration of the user.

we are patching this "custom" configuration of the user if it is a CameraConfiguration but not when it's a dict.

anyways, I'll put in a commit to test the changes that can be made from what I've understood so far and see if the CI is happy

[pantheraleo-7]

I would recommend changing the behaviour of the show_preview parameter.

Currently:
False -> NullPreview
None -> no event loop

I propose:
None -> NullPreview
False -> no event loop

(this will obviously be a breaking change)

Also change the default value of this parameter in all the methods to None (i.e. NullPreview) as default values should not be opinionated

[davidplowman]

Thanks for the update. I'm always nervous about changing existing behaviour, but maybe what you propose is OK. I guess the usual idioms we find are

• show_preview=True = just try to use a windowing preview, any windowing preview, that will work
• show_preview=Preview.QT = use the Preview.QT window (in this example). Could be any other preview type.
• leave at default = use NullPreview

So I guess if all those work the same, that probably sounds fair, and we can flag any more subtle change in a release note.

How far along do you think you are with this, are you wanting to get it merged at this point? Thanks.

[pantheraleo-7]

This PR is ready, I'll put in a separate PR for the show_preview proposal.