[libcamera-devel] [PATCH] Add enable_auto_focus option to the GStreamer plugin

[Laurent Pinchart]

Hi Cedric,

On Fri, Jun 02, 2023 at 03:53:03PM +0200, Cedric Nugteren wrote:
> I have fixed the review comments and have installed and configured git
> send-email.

Thank you. That appears to have gone well as your new version hasn't
been mangled by mail clients and/or servers :-)

> However, apparently it started a new thread, I don't think that
> is what it should have done:
> https://lists.libcamera.org/pipermail/libcamera-devel/2023-May/037925.html
> (I used "git send-email --in-reply-to=037947 --to=
> libcamera-devel at lists.libcamera.org patch_file.patch")

The --in-reply-to argument takes a Message-ID. The 037947 number you've
used is the index from the mailing list archives. The Message-ID of your
original e-mail was
CAM6JzGDQMvPaOJfvUGy286PhM5oUvyxSkWWq8YFvmKoW1T0Rfw at mail.gmail.com (you
can find it by checking the mail headers in your sent folder, or in
patchwork at https://patchwork.libcamera.org/patch/18651/).

This being said, new versions of a patch or patch series are not
supposed to be sent as a replies to the previous version, so all is fine
:-)

> Also, you mentioned that would like to see the original patch better
> formatted. Should I just create a whole new patch altogether?

When submitting a new version of a patch, you should, well, submit a new
version of the patch :-) The patch you have sent, titled "[PATCH] Apply
review suggestions" is not a new version of the original patch, but a
set of changes that apply on top. As the original patch won't be merged,
you should instead send a v2 stemming from v1 and incorporating the
changes requested during review.

In git's terms, that means that the changes you've made to v1 should be
squashed into the original commit (`git commit --amend`), not committed
as a separate commit. When generating the patch with git-format-patch,
you can pass the `-v2` argument to record the version number in the
subject line (you can also do so manually by editing the patch file, but
that's more error-prone).

A nice thing to do when sending new versions is to include a changelog.
See https://patchwork.libcamera.org/patch/18699/ for instance, and note
the 'Changes since v1:' section under the '---'. This is something you
can include directly in the commit message in your git tree, by adding a
'---' marker below the tags (Signed-off-by & co.), and recording the
changelog there. This helps reviewers understanding what has changed
between patch versions.

I've marked "[PATCH] Apply review suggestions" as "changes requested" in
patchwork. Could you submit a real v2 of this patch ?

> Finally, about the default/non-default discussion, I was trying to be
> conservative here, because when I opened the original bug (#188), the reply
> I got sounded like you thought it was a missing feature rather than a bug.
> For me this is a blocking bug: I can't use the PiCamV3 at all due to random
> focus behaviour. So therefore I opted for keeping the default behaviour as
> it was before to make it more likely that this patch will be accepted.

I'm tempted to default to disabling auto-focus. The discussion is still
ongoing though, but that shouldn't block you. You can send a v2 with
auto-focus disabled by default, and we can easily switch the default on
top if desired.

> On Fri, Jun 2, 2023 at 7:19 AM Laurent Pinchart wrote:
> > On Thu, Jun 01, 2023 at 12:24:22PM -0400, Nicolas Dufresne wrote:
> > > Le jeudi 01 juin 2023 à 08:39 +0100, Naushir Patuck a écrit :
> > > > > If you have any idea what the default should be in the GStreamer
> > > > > element, please feel free to chime in :-)
> > > >
> > > > Perhaps GStreamer should do nothing with focus by default and rely on the IPA
> > > > default behaviour (i.e. go to hyperfocal)?  This then relies on the application
> > > > to choose what it wants to do on startup, just like any other application that
> > > > directly uses libcamera.
> > >
> > > To reassure you, it can't be worse then what most UVC camera do today. Having a
> > > default matching this would help consistency though (or being similarly bad from
> > > some point of view). Later, on top, there should be ways to move to a more
> > > manual control in focus aware Gst applications.
> >
> > Hmmmm... I'm really in two minds here.
> >
> > First of all, we can't expect the same default behaviour for all cameras
> > when it comes to auto-focus, simply because many cameras don't have a
> > controllable focus lens, and some (I'm thinking about UVC devices here)
> > may not have the ability to turn auto-focus on. Still, implementing
> > consistent defaults where possible is a goal I highly value, in order to
> > maximize application portability.
> >
> > When it comes to the libcamera core, setting the focus lens to the
> > hyperfocal distance and disabling auto-focus seems like a good default
> > to me. Different use cases would favour different defaults, and I don't
> > think there's one particular use case that could be considered so
> > prevalent that it would call for enabling continuous auto-focus by
> > default. Different defaults could however be implemented in different
> > layers. For instance, I could imagine PipeWire deciding to enabling
> > continuous auto-focus by default on desktop or mobile systems.
> >
> > For libcamerasrc, I don't know if the same reasoning should lead to the
> > same result, or if we should consider that the vast majority of use
> > cases for the GStreamer element call for enabling auto-focus by default.
> > I currently side towards doing the same as in the libcamera core, as I
> > believe it would be confusing for users to have different default
> > behaviours when using libcamera through different adaptation layers.
> > However, I could also imagine enabling auto-focus by default in the V4L2
> > adaptation layer for instance, as that layer is mostly meant to expose
> > libcamera-based cameras as webcams.

-- 
Regards,

Laurent Pinchart