Improve user experience around previews

[pantheraleo-7]

based on #1248(comment), #1251(comment)

Overview:

• Deprecate parameter show_preview in favour of preview across all methods.
• Deprecate bool type as a value to the preview parameter.
• Add a member NO to the Preview enum. It skips starting an event loop (replaces behaviour of show_preview=None).
• Add a static method auto() to the Preview enum. It returns the [auto-]detected preview from Preview.{NULL,DRM,QT,QTGL} (replaces deprecated [show_]preview=True).
• Make preview=None the 'future' default across all methods, in order to unify the default behaviour. It starts a NullPreview (replaces deprecated [show_]preview=False).
• Add logging & documentation about this future change to the affected start_and_capture_file() & start_and_capture_files() (currently they default to auto-detection i.e. [show_]preview=True).

apart from the usual 'more pythonic, readable, idiomatic' and the obvious gains in separation of concerns (SoC), the most significant improvement is that users can now know, ahead of time, which preview will be detected by calling Preview.auto() and storing the result

preview = Preview.auto()
print(preview)

If preview is not specified, show_preview will take effect hence maintaining backwards compatibility.

To migrate, users shall use preview with a non-deprecated valid value or omit both preview & show_preview to get the default behaviour.

To get the future default behaviour for start_and_capture_file() & start_and_capture_files() now, users must explicitly pass preview=Preview.NULL.

[pantheraleo-7]

for some reason smoke test pi4 is queued perennially, even before I force pushed anything

[pantheraleo-7]

these class-level constants provide simple one-to-one aliases to constants that can be accessed at the top-level of a std-lib module - totally redundant. plus they don't belong to a class representing a "camera". and they are not used even once in the entire codebase, I have doubts that end-users use them. I'll recommend removing them. the "convenience" it provides doesn't really make sense.

users can just import logging

[davidplowman]

Yes, that unit seemed to have become completely unresponsive and I actually had to go and physically power-cycle it. No idea why. Anyway, seems to have come back to life now!

[davidplowman]

Hi, thanks for this PR.

I like the idea of Preview.autio() and this seems like a good change.

Can you just comment on the use of show_preview=True (or False) that is currently quite common. Is this something we can still support (even if we are moving away from it in future)? I'd be very nervous about a change that would force large numbers of users to change existing code. Thanks!

[pantheraleo-7]

Since we're in beta I didn't add a deprecation warning. Do we have a formal deprecation policy?

If you think this is a good idea, I can add it.

[davidplowman]

So Picamera2 has been stuck "in beta" forever now (like some other projects!), I think what matters is that it has probably 100s of very active users, and maybe 1000s overall.

I can't say there's a "formal" policy about changing APIs, except that, given the above, it's not a thing to do lightly. Having said that, I'm still keen to move forward and adopt improvements, so long as we can do things like:

• Add the improved behaviour without breaking the old behaviour.
• At some point, we can add a warning where people are relying on the old behaviour, telling them they need to change.
• Then, once folks have had a window of time to make such changes, then I think we can consider removing the old behaviour entirely.

Sorry if that sounds a bit fussy, but hopefully we can manage something like that?

Also just a heads-up that we're rebasing our libcamera back onto the upstream version which has had some binary/API incompatible changes. I've updated the Picamera2 next branch to cope with them, but just be warned that over the next couple of days there may be issues like Picamera2 next not working on our current libcamera release, or the libcamera main branch. But hopefully we'll resolve all this and get everything updated before the end of the week.

[pantheraleo-7]

I already added backwards compatibility to the deprecation of show_preview=True (and False) yesterday

But I don't see a meaningful way to "deprecate" the change in behaviour of show_preview=None (and False) except a release note documenting it

Then, once folks have had a window of time to make such changes, then I think we can consider removing the old behaviour entirely.

Which will be how many release cycles or weeks/months?

[davidplowman]

Hi, yes this is all a bit tricky. That preview argument has way too many permutations.

I wonder if the answer is to deprecate and replace the old parameters. So for example, start() takes a show_preview parameter. Maybe we add a preview parameter which does what we now want, and issue a warning if it's not used. Later, we can remove the old one.

There's still an issue with using start() with only default parameters which gives you no event loop at all, so maybe we need to preserve that behaviour? Obviously this all needs to be thought about carefully!

As regards the timescales, I think we need a minimum of one release where the old behaviour works but is clearly deprecated, so I think that might mean "a couple of months", or something like that. I think folks will come with us if the new scheme is clearly more logical and better, and clearly signalled. I don't think I could handle the backlash caused by breaking stuff suddenly and without warning!!

Does that make some sense? Sorry that this is all quite so tricky.

[pantheraleo-7]

Initially I wanted to rename show_preview to preview (to unify the API across all the methods) but seeing you were not a fan of breaking changes I never mentioned it haha because it seemed too destructive of a change.

So I instead focused on the values of show_preview because they were more messy (for starters, start and start_preview behave differently for [show_]preview=None).

Does that make some sense? Sorry that this is all quite so tricky.

Yes, you make sense to me. Rather I would want you to say what you want so that we can expedite things.

Thanks for being nice till now.

(Check the PR description for the new changes)

[davidplowman]

Hi again, I think I'm confusing myself too. Let me try and step back and double check what we want to do here.

So firstly, we're OK with passing an enum (e.g. Preview.NULL) to start() as the show_preview parameter.

We're also OK with passing an actual preview instance, which should also let us have Preview.auto() to find out what event loop type it will try to choose (if we want an actual window). It also, in theory anyway, would let people define their own previews.

We want to deprecate the use of True (same as Preview.auto()) or False (same as Preview.NULL). We note that False is the default currently.

(There's also a slight weirdness about None being different in start() and start_preview(), though calling start_preview() when you don't want an event loop at all doesn't make too much sense!)

Anyway, does that all sound correct so far?

[pantheraleo-7]

Anyway, does that all sound correct so far?

Yes

Let me reiterate lest we miss something..

Old behaviour (show_preview):
None -> no preview
False -> NullPreview (default)
True -> auto-detect

New behaviour (preview):
None -> NullPreview (default)
Preview.NO -> no preview
Preview.auto() -> auto-detect

As for start_preview, to maintain a consistent API, passing Preview.NO is a no-op!

Unless users opt to migrate by explicitly using the preview parameter, there will be no behavioural changes, except for the deprecation warnings, in this PR.

Once the deprecation period ends, the following changes will take place through a separate PR:

• show_preview will no longer exist
• True & False will no longer be accepted as values
• Default value of start_and_capture_file() & start_and_capture_files() will change from auto-detection to NullPreview

[davidplowman]

Thanks for the confirmation there. And just to understand completely, what would the difference be if we just kept show_preview and didn't replace it with preview? (I know I suggested it, just want to check!!)

The start_and_capture_file() function (and friends) is an interesting one. It doesn't really belong in Picamera2 at all, I think, it was put there at the request of the part of the organisation interested with primary school education. They wanted simple "one-liners" for small children to type that would show them camera pictures and capture images. I wonder if we can preserve the default "auto" behaviour for those? They are a niche category IMHO - so I don't mind if they behave differently.

[pantheraleo-7]

They are a niche category IMHO - so I don't mind if they behave differently.

Ok. Then why not start_and_record_video? It defaults to NullPreview... (I want a consistent API that's all xD)

if we just kept show_preview and didn't replace it with preview?

Then we won't be able to deprecate (i.e. phase out) the old behaviour. We'll have to remove it altogether in a given release.