[{"id":25896,"web_url":"https://patchwork.libcamera.org/comment/25896/","msgid":"","date":"2022-11-24T13:43:34","subject":"Re: [libcamera-devel] [PATCH 4/9] libcamera: camera_sensor: Verify\n\tflips support","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the patch.\n\nOn Thu, 24 Nov 2022 at 12:12, Jacopo Mondi wrote:\n>\n> During the camera sensor driver validation, verify if the sensor\n> supports horizontal and vertical flips and store a flag as\n> CameraSensor::supportFlips_ class member.\n>\n> The flag will be later inspected when applying flips.\n>\n> Signed-off-by: Jacopo Mondi \n\nI'm certainly fine with this, though I should perhaps add that when we\noriginally did this in the Pi PH we discussed a little bit whether a\ncamera might support H flips but not V, or vice versa. We slightly\npassed over the question because it was \"only the Pi\", but maybe we\nwant to re-examine that?\n\nThere's also the question as to whether the flips modify the Bayer\norder. Don't know if that's a thing we want to store here? (And\nseparately for H and V flips?)\n\nBut as I said previously, this all works for me, so:\n\nReviewed-by: David Plowman \n\nThanks!\nDavid\n\n> ---\n> include/libcamera/internal/camera_sensor.h | 1 +\n> src/libcamera/camera_sensor.cpp | 15 ++++++++++++++-\n> 2 files changed, 15 insertions(+), 1 deletion(-)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index b9f4d7867854..878f3c28a3c9 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -101,6 +101,7 @@ private:\n> Size pixelArraySize_;\n> Rectangle activeArea_;\n> const BayerFormat *bayerFormat_;\n> + bool supportFlips_;\n>\n> ControlList properties_;\n>\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index cbac9e7801ae..3afcbc482095 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -57,7 +57,8 @@ LOG_DEFINE_CATEGORY(CameraSensor)\n> */\n> CameraSensor::CameraSensor(const MediaEntity *entity)\n> : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),\n> - bayerFormat_(nullptr), properties_(properties::properties)\n> + bayerFormat_(nullptr), supportFlips_(false),\n> + properties_(properties::properties)\n> {\n> }\n>\n> @@ -271,6 +272,18 @@ int CameraSensor::validateSensorDriver()\n> }\n> }\n>\n> + /* Verify if sensor supports horizontal/vertical flips. */\n> + const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP);\n> + const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP);\n> + if (hflipInfo && vflipInfo &&\n> + !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) &&\n> + !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY))\n> + supportFlips_ = true;\n> +\n> + if (!supportFlips_)\n> + LOG(CameraSensor, Warning)\n> + << \"Camera sensor does not support horizontal/vertical flip\";\n> +\n> /*\n> * Make sure the required selection targets are supported.\n> *\n> --\n> 2.38.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C3D33BE08B\n\tfor ;\n\tThu, 24 Nov 2022 13:43:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0BB936331A;\n\tThu, 24 Nov 2022 14:43:49 +0100 (CET)","from mail-pf1-x42d.google.com (mail-pf1-x42d.google.com\n\t[IPv6:2607:f8b0:4864:20::42d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F80A632EA\n\tfor ;\n\tThu, 24 Nov 2022 14:43:47 +0100 (CET)","by mail-pf1-x42d.google.com with SMTP id b4so1652898pfb.9\n\tfor ;\n\tThu, 24 Nov 2022 05:43:47 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669297429;\n\tbh=Y2uNEMarPyIkVq5zwkzELnhQX6d+zCqnFtEYnahNRdY=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=JYTS8DMdbRBE9hOvBOo6f59ZVlNdE20ywwS5BdcrhI+L3z0yvgrgzCCC9AyTQ/yAL\n\tbh2EEXYMV6zfGR3Job+LMkdAzqp29gQwYKRpwjqCZTU8UGHXkXKZX4kfHQY5KdOfkh\n\tMFgHhktrVBY79K0JKFM8t1GVJfHTaeufwq3pMRlRxdt4PhWirOP1IhhU36S05rXTXt\n\t7JkhWkenu0uuB+/+zfwIIpzrHO+Tpu1KCC/HunW+4oNv9i2iOppUoefLHyeDekuL9X\n\tMTZxBIHXdW3fbMiKOpP8Kh/s0yHfVq73+ra4nc/DvjpJBVtk9/kp8ob+k6viQwjKXq\n\ttzcPxMeSo30LA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=15Uksxi6bJg0CElpBqGtYOkX6//hxCiNs6eXn3s2Zw4=;\n\tb=pTbL2m/BS8velar/qp6gHRW9OYQ4W7CoUKzh4TVtPwYmXgM71RxV6dl1yRo61m8q9A\n\tJCb4aiUySptiZJOyyPXAO8fUNVargqZSGCzlemVO0zdNJAEpN4Xw0c92YSqQ3qCWPa5a\n\tICHMPeZHvWYUMrnxdIP18Dtp+SFoijq3fEe+P3JtdXXIonUXg+c9C/znZLYl8HEye6J9\n\tEysXLnzDVcUDrnMNkIIqIq9liS/s/Mhsu1xY2FrMgUAfabNIutunMb1st14T+4e3ZZZM\n\tP9tts7EvLHvL4iqWSBetBisp14vkzm4a4t+pItkwsjvjtlcW2l81aa55MRXElOYYs6S2\n\tcOBQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"pTbL2m/B\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=15Uksxi6bJg0CElpBqGtYOkX6//hxCiNs6eXn3s2Zw4=;\n\tb=kkcOwygFOW7lusS86HfXJP62ald7VFen1MDfn/4g14Ew3CFuC/6zvXqqLkgJp++fvB\n\t1fWjxikajfEPxCbRgpfUxiwd2F2Ucs0jgRbDgN4xyhHVvPo8KotTKsHorFR7T/QrYSIQ\n\tF4FzcKQ1SytgieNjzdBSqsqyMiltB3R8Tz7cweMyTvxGnzFRjcW+Umk0D1fMhL9Tf8Ul\n\tCKTV2bJe/g9bOkBGRasSOWifwAXMpQGhINoWTyatoAq5emqvgGROY81PKuuVOK2aiaYE\n\t344vV/c7N7IQVoIzQKwV/MrRyQzPYjfycO64fEP2K068ucqEjzMbrrwqFZG1d53ah7dF\n\tyahg==","X-Gm-Message-State":"ANoB5pnAjsULFMX4xEMFoY9XShdtdqwDo2DZWuqHzHhK1QZ1eNRQot4v\n\tGObQ9f5K+bj04FR+LWe6VNLpTIi3+Uy5Ndy4XhN4Sw==","X-Google-Smtp-Source":"AA0mqf7Z5GSerqARKj9fvfA8J7FV5k/FPyva+xShfpu/GS5Ghi5m8s3MrCmfprWr8yBUyHpXEgBLWpajVg4vwkimrdM=","X-Received":"by 2002:a63:cc48:0:b0:477:5515:8a9c with SMTP id\n\tq8-20020a63cc48000000b0047755158a9cmr11711800pgi.256.1669297425795;\n\tThu, 24 Nov 2022 05:43:45 -0800 (PST)","MIME-Version":"1.0","References":"<20221124121220.47000-1-jacopo@jmondi.org>\n\t<20221124121220.47000-5-jacopo@jmondi.org>","In-Reply-To":"<20221124121220.47000-5-jacopo@jmondi.org>","Date":"Thu, 24 Nov 2022 13:43:34 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 4/9] libcamera: camera_sensor: Verify\n\tflips support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25898,"web_url":"https://patchwork.libcamera.org/comment/25898/","msgid":"<20221124140523.lm7xuohxcdlaywbf@uno.localdomain>","date":"2022-11-24T14:05:23","subject":"Re: [libcamera-devel] [PATCH 4/9] libcamera: camera_sensor: Verify\n\tflips support","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David\n\nOn Thu, Nov 24, 2022 at 01:43:34PM +0000, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for the patch.\n>\n> On Thu, 24 Nov 2022 at 12:12, Jacopo Mondi wrote:\n> >\n> > During the camera sensor driver validation, verify if the sensor\n> > supports horizontal and vertical flips and store a flag as\n> > CameraSensor::supportFlips_ class member.\n> >\n> > The flag will be later inspected when applying flips.\n> >\n> > Signed-off-by: Jacopo Mondi \n>\n> I'm certainly fine with this, though I should perhaps add that when we\n> originally did this in the Pi PH we discussed a little bit whether a\n> camera might support H flips but not V, or vice versa. We slightly\n> passed over the question because it was \"only the Pi\", but maybe we\n> want to re-examine that?\n\nI think we should.\n\nI recall I had a \\todo somewhere in these patches to split H/V flip\nhandling, but seems like I removed it.\n\nAs this is what we have already I would do this on top, as it would\nrequire to modify the newly introduced\nCameraSensor::validateTransform() as well as the new logic in\nsetFormat().\n\n>\n> There's also the question as to whether the flips modify the Bayer\n> order. Don't know if that's a thing we want to store here? (And\n> separately for H and V flips?)\n\n-Ideally- this should be transparent for us. What I mean is that,\nassuming a sensor driver that behaves \"correctly\" with:\n\n- RW CID_VFLIP/CID_HFLIP\n- V4L2_CTRL_MODIFY_LAYOUT for both controls\n- Updates the bayer pattern order when a new flip is applied\n\nWith your #2 patch we retrieve the \"native\" (no flips) Bayer order.\nWhen a new format is set on the CameraSensor flips are applied before\ncalling V4L2Subdevice::setFormat() and the format returned from this\nfunction contains the correct mbus code, and we use this to configure\nthe rest of the pipeline.\n\nAll of this assumes that the sensor is configured first, then the rest\nof the pipeline, but I think it's a fair assumption, and it's not\ndifferent than what happens with the image format.\n\nSpeaking of this: I saw this on your pipeline handler validate()\n\n /*\n * Some sensors change their Bayer order when they are\n * h-flipped or v-flipped, according to the transform.\n * If this one does, we must advertise the transformed\n * Bayer order in the raw stream. Note how we must\n * fetch the \"native\" (i.e. untransformed) Bayer order,\n * because the sensor may currently be flipped!\n */\n V4L2PixelFormat fourcc = unicamFormat.fourcc;\n if (data_->flipsAlterBayerOrder_) {\n BayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);\n bayer.order = data_->nativeBayerOrder_;\n bayer = bayer.transform(combinedTransform_);\n fourcc = bayer.toV4L2PixelFormat();\n }\n\nThat's probably something we want on the other pipelines too, and I\nwould like to avoid caching in each pipeline flipsAlterBayerOrder_ and\nnativeBayerOrder_ and delegate this to some shared helper, again in\nthe CameraSensor class ?\n\n>\n> But as I said previously, this all works for me, so:\n>\n> Reviewed-by: David Plowman \n\nThanks\n j\n\n>\n> Thanks!\n> David\n>\n> > ---\n> > include/libcamera/internal/camera_sensor.h | 1 +\n> > src/libcamera/camera_sensor.cpp | 15 ++++++++++++++-\n> > 2 files changed, 15 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index b9f4d7867854..878f3c28a3c9 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -101,6 +101,7 @@ private:\n> > Size pixelArraySize_;\n> > Rectangle activeArea_;\n> > const BayerFormat *bayerFormat_;\n> > + bool supportFlips_;\n> >\n> > ControlList properties_;\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index cbac9e7801ae..3afcbc482095 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -57,7 +57,8 @@ LOG_DEFINE_CATEGORY(CameraSensor)\n> > */\n> > CameraSensor::CameraSensor(const MediaEntity *entity)\n> > : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),\n> > - bayerFormat_(nullptr), properties_(properties::properties)\n> > + bayerFormat_(nullptr), supportFlips_(false),\n> > + properties_(properties::properties)\n> > {\n> > }\n> >\n> > @@ -271,6 +272,18 @@ int CameraSensor::validateSensorDriver()\n> > }\n> > }\n> >\n> > + /* Verify if sensor supports horizontal/vertical flips. */\n> > + const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP);\n> > + const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP);\n> > + if (hflipInfo && vflipInfo &&\n> > + !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) &&\n> > + !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY))\n> > + supportFlips_ = true;\n> > +\n> > + if (!supportFlips_)\n> > + LOG(CameraSensor, Warning)\n> > + << \"Camera sensor does not support horizontal/vertical flip\";\n> > +\n> > /*\n> > * Make sure the required selection targets are supported.\n> > *\n> > --\n> > 2.38.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 06FBDBDE6B\n\tfor ;\n\tThu, 24 Nov 2022 14:05:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 765716331F;\n\tThu, 24 Nov 2022 15:05:26 +0100 (CET)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D3AF26330D\n\tfor ;\n\tThu, 24 Nov 2022 15:05:24 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 3BFD71C000E;\n\tThu, 24 Nov 2022 14:05:23 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669298726;\n\tbh=naLPjLJrxP0c25OmLkpEvz+P/2kVddZvzJ6S8uSJsFw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=p5OLEC8tJSEY99t4N0eipDJrQqThIzh7nTNbqpoojNwu89GaXIbJHLtHFOaV5fclX\n\t0I3thazLwloFvKPfWt0Kzi6ph48xVQY9c0ib7S5FhM3GtGYTsncLLPeMkJtvfRCWr1\n\tZp7ZFxyeeWF0ZeLd6AbncbuE/UE9dYrDFvMoVSpVtjmPAAVQzf5T6g1cEvGSLlbWxm\n\to4U2u/uhZOQXnFyq95jsesxuahd94PjP6DZF20w7oae/8y070+QM3heChakeLSDfO6\n\t4KlxwhSZXl3mUdSvUg80j73zFkVSevBOK/MvTUAFq56Wdb2Wpta9dfRN3+ptGxspwg\n\tpKKfGXpPsHi2Q==","Date":"Thu, 24 Nov 2022 15:05:23 +0100","To":"David Plowman ","Message-ID":"<20221124140523.lm7xuohxcdlaywbf@uno.localdomain>","References":"<20221124121220.47000-1-jacopo@jmondi.org>\n\t<20221124121220.47000-5-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 4/9] libcamera: camera_sensor: Verify\n\tflips support","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]