[{"id":25811,"web_url":"https://patchwork.libcamera.org/comment/25811/","msgid":"<20221117103210.lum6d2ywokoimqxe@uno.localdomain>","date":"2022-11-17T10:32:10","subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: pipeline: uvcvideo:\n\tSet device's flip controls correctly","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 10, 2022 at 02:45:54PM +0000, David Plowman via libcamera-devel wrote:\n> Set the horizontal and vertical flip controls correctly by\n> calling the device's setTransform method now that this no longer\n> happens in CameraSensor::init().\n>\n> Signed-off-by: David Plowman \n> ---\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 277465b7..4a891c23 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -209,6 +209,9 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n> \tStreamConfiguration &cfg = config->at(0);\n> \tint ret;\n>\n> +\t/* Set up the video device's horizontal and vertical flips. */\n> +\tdata->video_->setTransform(config->transform);\n> +\n\nDo we need this ? The validate() function does\n\n\tif (transform != Transform::Identity) {\n\t\ttransform = Transform::Identity;\n\t\tstatus = Adjusted;\n\t}\n\nSame for simple and rkisp1.\n\nThis opens a different question. What happens if another application\nsets a flip before libcamera is started ? Should we here instead\nreset all flips to their default values ? Using the newly introduced\nsetTrasform(Identity) wouldn't be enough as it would not clean up any\nexternally set flip...\n\nWhat would the expected configuration from the application point of\nview ? Whatever flip is externally set is kept, or all flips\nare reset to their default ? Please note that \"reset to their\ndefaults\" != \"clear any flip flag\" as most drivers (and we discussed\nthis with Dave recently) do inizialize their flip controls to report\nwhat is actually programmed in registers, and quite often sensors are\n180 degrees flipped by default:\nhttps://lore.kernel.org/all/CAPY8ntCCpMZtG-eJQ8cgd_GO06ZDP32oRK8mGrZKGMU2W401Dg@mail.gmail.com/\nThis is one more reason to remove the \"set flips to 0\" in init.\n\nDo we need a V4L2Device::resetFlips() function instead ?\n\n> \tV4L2DeviceFormat format;\n> \tformat.fourcc = data->video_->toV4L2PixelFormat(cfg.pixelFormat);\n> \tformat.size = cfg.size;\n> --\n> 2.30.2\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 4FAF0BE08B\n\tfor ;\n\tThu, 17 Nov 2022 10:32:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6936B6309B;\n\tThu, 17 Nov 2022 11:32:19 +0100 (CET)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 47E8D61F34\n\tfor ;\n\tThu, 17 Nov 2022 11:32:18 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id ADC2BE0008;\n\tThu, 17 Nov 2022 10:32:16 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668681139;\n\tbh=ztA6dovAKVfWRzx1hNucXr+rl6Ud8Av2XTxzN2PiI+U=;\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=DFPxB9dP0upc/AgNAw/SiMHKLTyH6g1zBvld7tzvLnlsXOccHFQXllqeW3XyBaOim\n\tsqxWspF2BP4IeduO0JOipBVmQilICeoFO5zp5SMjmmzaVxYmTv3H9uBBpUiAOAMSgu\n\tAQD2k2cDtbiGuSExcQ81sb7Jt/kALUO3GIOW+8zpiIE55dAARsFunhNDD0N+UtXMOO\n\tn++YOnsjZADxzd3jFMTDgtGL46nEVm7JZ9FeYVvV4lMgUtqQZh8tK7VXC0+obIAnK0\n\tq22+fyGzKE2FyrKQ7AK4EoFihWyKda+kKtR/KeR06BY0H+6FLJKeIi160uN4PooFyK\n\tp8A9x4F4xM9SA==","Date":"Thu, 17 Nov 2022 11:32:10 +0100","To":"David Plowman ","Message-ID":"<20221117103210.lum6d2ywokoimqxe@uno.localdomain>","References":"<20221110144556.7858-1-david.plowman@raspberrypi.com>\n\t<20221110144556.7858-5-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221110144556.7858-5-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: pipeline: uvcvideo:\n\tSet device's flip controls correctly","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,\n\tDave Stevenson ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25814,"web_url":"https://patchwork.libcamera.org/comment/25814/","msgid":"","date":"2022-11-17T13:02:29","subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: pipeline: uvcvideo:\n\tSet device's flip controls correctly","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 comments!\n\nOn Thu, 17 Nov 2022 at 10:32, Jacopo Mondi wrote:\n>\n> Hi David\n>\n> On Thu, Nov 10, 2022 at 02:45:54PM +0000, David Plowman via libcamera-devel wrote:\n> > Set the horizontal and vertical flip controls correctly by\n> > calling the device's setTransform method now that this no longer\n> > happens in CameraSensor::init().\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 +++\n> > 1 file changed, 3 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 277465b7..4a891c23 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -209,6 +209,9 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n> > StreamConfiguration &cfg = config->at(0);\n> > int ret;\n> >\n> > + /* Set up the video device's horizontal and vertical flips. */\n> > + data->video_->setTransform(config->transform);\n> > +\n>\n> Do we need this ? The validate() function does\n>\n> if (transform != Transform::Identity) {\n> transform = Transform::Identity;\n> status = Adjusted;\n> }\n>\n> Same for simple and rkisp1.\n>\n> This opens a different question. What happens if another application\n> sets a flip before libcamera is started ? Should we here instead\n> reset all flips to their default values ? Using the newly introduced\n> setTrasform(Identity) wouldn't be enough as it would not clean up any\n> externally set flip...\n>\n> What would the expected configuration from the application point of\n> view ? Whatever flip is externally set is kept, or all flips\n> are reset to their default ? Please note that \"reset to their\n> defaults\" != \"clear any flip flag\" as most drivers (and we discussed\n> this with Dave recently) do inizialize their flip controls to report\n> what is actually programmed in registers, and quite often sensors are\n> 180 degrees flipped by default:\n> https://lore.kernel.org/all/CAPY8ntCCpMZtG-eJQ8cgd_GO06ZDP32oRK8mGrZKGMU2W401Dg@mail.gmail.com/\n> This is one more reason to remove the \"set flips to 0\" in init.\n>\n> Do we need a V4L2Device::resetFlips() function instead ?\n\nThat's all interesting. Indeed I see there's a problem with devices\nthat might have non-zero defaults, or where someone else has changed\nthem without us being aware.\n\nAs regards uvcvideo, maybe the best thing is then just to *do\nnothing*? At least that preserves the current behaviour. Although the\npipeline handler doesn't support flips, someone could actually set\nthem externally and they would be preserved. If the pipeline handler\never does support flips (should it one day?) we can figure out how it\nmight work then.\n\nProbably the same argument holds for the simple PH too?\n\nI wonder if rkisp1 is different. That uses the CameraSensor class, so\npreviously I assume it was clearing the flip bits. Therefore resetting\nthe transform (it's always the Identity) might be correct? If a change\nof behaviour is necessary (actually handling the transforms would be\nideal!), that's probably a separate piece of work.\n\nThoughts?\n\nDavid\n\n>\n> > V4L2DeviceFormat format;\n> > format.fourcc = data->video_->toV4L2PixelFormat(cfg.pixelFormat);\n> > format.size = cfg.size;\n> > --\n> > 2.30.2\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 AC87ABD16B\n\tfor ;\n\tThu, 17 Nov 2022 13:02:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2639861F32;\n\tThu, 17 Nov 2022 14:02:45 +0100 (CET)","from mail-pg1-x52f.google.com (mail-pg1-x52f.google.com\n\t[IPv6:2607:f8b0:4864:20::52f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5854361F31\n\tfor ;\n\tThu, 17 Nov 2022 14:02:42 +0100 (CET)","by mail-pg1-x52f.google.com with SMTP id q71so1890275pgq.8\n\tfor ;\n\tThu, 17 Nov 2022 05:02:42 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668690165;\n\tbh=Zsal6yHwkZuRStjxNl5lOuH4NuGnulipsNJdFHitVuE=;\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=QTV3qhfEYVsJp83YrhDVmdk5bef7mcETpezhqPyL7OZHWpqxaTN7LFiGSJnzIgOF/\n\tmEIKfn1bgKRq5AICz6tUZvt3nyZejvMlQeDx3JO/6LHyrP3R56AP2WU1Yh3RHr3Z3C\n\t6/xSdzNuFWwi9/lXk70TctP2Gflsk0u5o+3T8UZaxaVhC8az4B5hpj9ay1xfQVQaqa\n\tF0kgRe7LGG+nV4o99DhQQmBk1aPzZ/lbQUijKzvrlDldz7kxX76OqZ7aJYKJTUMgH5\n\tiCi3VWAP/2R7wwZs8Zu/bqLi0PYPKd7WSWomuSrs+tZUuSnw8lt7eTZ0XqlKQ17ywr\n\tV8/cV1ROA/LNw==","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=jhzHdtfIkBgsn88y97dZDDgieZEkKJijNpl41BX8/mw=;\n\tb=BjxtkDaFvOaQqaNo20rU7edmOEnaUCYciiy9SFLXcKaDO8zjIHCOcf5LauWwMLApoy\n\tut3iDmISx17TazEJ4bFADQIJJLF+IoIfEn5zjF/y1b2N54KJjjyrtENiAZuV0JoaeiBr\n\twhuyK3Su2hRIq09QjfxiW05Y+acsShe8yiU7qzQXl1Sez6DkClxrXHFPXgeE3jsiX7Tv\n\tNqr57dS0At8ECzc+FeAhs4aDlotW/4gOPN/A1RXvpUHl/ctGQeVymTsIwXYlJ+kYvRzl\n\tzcKbdetQpilhAWp84Pa9Qw8HhhsY4R/PRD5d41JwiM094KGoy2yTjaHDsz6Z/EoRA0pS\n\twDKQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"BjxtkDaF\"; 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=jhzHdtfIkBgsn88y97dZDDgieZEkKJijNpl41BX8/mw=;\n\tb=SvDKqbqfU0RXGyCYwUK/teMsIy/LqMMt6WFpbGg82CMob47Py3Lb01Co690cT3gNIs\n\twR3LW6qWfS5mJNe4ngoNIgAc/jdyAbH+fWNcNHT2CHDedDIo/EX1q5kcTMfoOaGAWc8Y\n\tc+Zk5zohLbZ4v4Fo2lnCG5TiG9P4dffvRJg/7hwEQB8fSHO2GFWv+N0OI98FT0B370XW\n\ttVkXGbWRyNVAsATi/xWg5JTwsPrT8ePX7801ho6qdL0mPKPbpxrMYQOfniamo6VvxADb\n\tZQ/oud9r8ttunzF4LNkSY9QRW5Vp3og0wBz+sUQth12AGTylzDkmjSuAlRSRId2mUnsw\n\tWysA==","X-Gm-Message-State":"ANoB5pnCzmJoNirynJ2pEMzXv3Z/b+Fnd4kwDeg2EpOjjxX0Girvy7u5\n\tD+hn/BND1BnKXJlWCp9SAk7N5xk17RG2Bk14R/jMsQ==","X-Google-Smtp-Source":"AA0mqf5MScT8mSSq5PJIr72GxXv66CmIHAB2691COLF0rUY87w8cv0aVoxFKjhYUWUyuxq2JC1TKkWevDIhuD/R+PW4=","X-Received":"by 2002:a63:c63:0:b0:476:c4e1:958b with SMTP id\n\t35-20020a630c63000000b00476c4e1958bmr1965997pgm.256.1668690160629;\n\tThu, 17 Nov 2022 05:02:40 -0800 (PST)","MIME-Version":"1.0","References":"<20221110144556.7858-1-david.plowman@raspberrypi.com>\n\t<20221110144556.7858-5-david.plowman@raspberrypi.com>\n\t<20221117103210.lum6d2ywokoimqxe@uno.localdomain>","In-Reply-To":"<20221117103210.lum6d2ywokoimqxe@uno.localdomain>","Date":"Thu, 17 Nov 2022 13:02:29 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: pipeline: uvcvideo:\n\tSet device's flip controls correctly","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,\n\tDave Stevenson ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25818,"web_url":"https://patchwork.libcamera.org/comment/25818/","msgid":"<20221117181702.cvgf3fxivlwiy4vu@uno.localdomain>","date":"2022-11-17T18:17:02","subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: pipeline: uvcvideo:\n\tSet device's flip controls correctly","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 17, 2022 at 01:02:29PM +0000, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for the comments!\n>\n> On Thu, 17 Nov 2022 at 10:32, Jacopo Mondi wrote:\n> >\n> > Hi David\n> >\n> > On Thu, Nov 10, 2022 at 02:45:54PM +0000, David Plowman via libcamera-devel wrote:\n> > > Set the horizontal and vertical flip controls correctly by\n> > > calling the device's setTransform method now that this no longer\n> > > happens in CameraSensor::init().\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 +++\n> > > 1 file changed, 3 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > index 277465b7..4a891c23 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > @@ -209,6 +209,9 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n> > > StreamConfiguration &cfg = config->at(0);\n> > > int ret;\n> > >\n> > > + /* Set up the video device's horizontal and vertical flips. */\n> > > + data->video_->setTransform(config->transform);\n> > > +\n> >\n> > Do we need this ? The validate() function does\n> >\n> > if (transform != Transform::Identity) {\n> > transform = Transform::Identity;\n> > status = Adjusted;\n> > }\n> >\n> > Same for simple and rkisp1.\n> >\n> > This opens a different question. What happens if another application\n> > sets a flip before libcamera is started ? Should we here instead\n> > reset all flips to their default values ? Using the newly introduced\n> > setTrasform(Identity) wouldn't be enough as it would not clean up any\n> > externally set flip...\n> >\n> > What would the expected configuration from the application point of\n> > view ? Whatever flip is externally set is kept, or all flips\n> > are reset to their default ? Please note that \"reset to their\n> > defaults\" != \"clear any flip flag\" as most drivers (and we discussed\n> > this with Dave recently) do inizialize their flip controls to report\n> > what is actually programmed in registers, and quite often sensors are\n> > 180 degrees flipped by default:\n> > https://lore.kernel.org/all/CAPY8ntCCpMZtG-eJQ8cgd_GO06ZDP32oRK8mGrZKGMU2W401Dg@mail.gmail.com/\n> > This is one more reason to remove the \"set flips to 0\" in init.\n> >\n> > Do we need a V4L2Device::resetFlips() function instead ?\n>\n> That's all interesting. Indeed I see there's a problem with devices\n> that might have non-zero defaults, or where someone else has changed\n> them without us being aware.\n>\n> As regards uvcvideo, maybe the best thing is then just to *do\n> nothing*? At least that preserves the current behaviour. Although the\n> pipeline handler doesn't support flips, someone could actually set\n> them externally and they would be preserved. If the pipeline handler\n> ever does support flips (should it one day?) we can figure out how it\n\nOn \"should it be one day\": I presume so, but I can't tell if there are\nuvc-specific reasons why this can't be made\n\n> might work then.\n\nI presume so. Again not a uvc expert\n\n>\n> Probably the same argument holds for the simple PH too?\n>\n> I wonder if rkisp1 is different. That uses the CameraSensor class, so\n> previously I assume it was clearing the flip bits. Therefore resetting\n> the transform (it's always the Identity) might be correct? If a change\n\nIt seems to me Simple uses CameraSensor as well, but yes, it's mostly\nintended to work with YUV/RGB sensors. Nothing prevents to use RAW\nsensors though. The idea is to have one day a GPU-based ISP\nimplementation that could be plugged to Simple, so RAW+Simple is a use\ncase we care about..\n\nMy concern is only about RAW sensors.\n\n- Assume a sensor driver reports BGGR with both v4l2 flips set to 1 and both\n report V4L2_CTRL_FLAG_MODIFY_LAYOUT.\n- With your patch we transform it to RGGB and report it as\n properties::ColorFilterArrangement (should we rename it btw?).\n- If any other user changes on filp it changes the bayer components\n ordering too and applications might get confused ?\n\nIs something we should be concerned ?\n\n> of behaviour is necessary (actually handling the transforms would be\n> ideal!), that's probably a separate piece of work.\n>\n> Thoughts?\n>\n> David\n>\n> >\n> > > V4L2DeviceFormat format;\n> > > format.fourcc = data->video_->toV4L2PixelFormat(cfg.pixelFormat);\n> > > format.size = cfg.size;\n> > > --\n> > > 2.30.2\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 BE297BE08B\n\tfor ;\n\tThu, 17 Nov 2022 18:17:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2674961F31;\n\tThu, 17 Nov 2022 19:17:06 +0100 (CET)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 936A461F31\n\tfor ;\n\tThu, 17 Nov 2022 19:17:04 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id D293F240003;\n\tThu, 17 Nov 2022 18:17:03 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668709026;\n\tbh=G7fg6UfW4vNldiPppLFBJhY0FYCiAqDOmviITVYg5Ew=;\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=O6sRccfA3M6ituQy5VrQyasgxvxRzOnnlomZm6qeKdYFK4YS5GUz+jxqFLqP1HOj2\n\toV1fUD05+ZOAc1KHMG5qIcCt3aLrXkbFGyxyD99jnOKSv0dybDlY6hCJ68kI1eOC96\n\ttJZucsYyII/lmHiUwcPU8SLzGmUS1iUn5YKPTv4wEktNeG2W0JniZIemvhYGNsAUJP\n\t0aG7Jn/KijQHhgblzhBylCrXOBTSwatpP29ixoQOMMdZBHqmP/SGmrOESS0yrRvASG\n\tYMSDMlxVDX+h8BZ2bXm5B96az9C//UeLyDbsnk0A4DIqnajljBSEmsr98Tg8bh4utF\n\tl7GqVg1gCwZtQ==","Date":"Thu, 17 Nov 2022 19:17:02 +0100","To":"David Plowman ","Message-ID":"<20221117181702.cvgf3fxivlwiy4vu@uno.localdomain>","References":"<20221110144556.7858-1-david.plowman@raspberrypi.com>\n\t<20221110144556.7858-5-david.plowman@raspberrypi.com>\n\t<20221117103210.lum6d2ywokoimqxe@uno.localdomain>\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/6] libcamera: pipeline: uvcvideo:\n\tSet device's flip controls correctly","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,\n\tDave Stevenson ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25822,"web_url":"https://patchwork.libcamera.org/comment/25822/","msgid":"","date":"2022-11-18T17:26:56","subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: pipeline: uvcvideo:\n\tSet device's flip controls correctly","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nOn Thu, 17 Nov 2022 at 18:17, Jacopo Mondi wrote:\n>\n> Hi David,\n>\n> On Thu, Nov 17, 2022 at 01:02:29PM +0000, David Plowman wrote:\n> > Hi Jacopo\n> >\n> > Thanks for the comments!\n> >\n> > On Thu, 17 Nov 2022 at 10:32, Jacopo Mondi wrote:\n> > >\n> > > Hi David\n> > >\n> > > On Thu, Nov 10, 2022 at 02:45:54PM +0000, David Plowman via libcamera-devel wrote:\n> > > > Set the horizontal and vertical flip controls correctly by\n> > > > calling the device's setTransform method now that this no longer\n> > > > happens in CameraSensor::init().\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 +++\n> > > > 1 file changed, 3 insertions(+)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > index 277465b7..4a891c23 100644\n> > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > @@ -209,6 +209,9 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n> > > > StreamConfiguration &cfg = config->at(0);\n> > > > int ret;\n> > > >\n> > > > + /* Set up the video device's horizontal and vertical flips. */\n> > > > + data->video_->setTransform(config->transform);\n> > > > +\n> > >\n> > > Do we need this ? The validate() function does\n> > >\n> > > if (transform != Transform::Identity) {\n> > > transform = Transform::Identity;\n> > > status = Adjusted;\n> > > }\n> > >\n> > > Same for simple and rkisp1.\n> > >\n> > > This opens a different question. What happens if another application\n> > > sets a flip before libcamera is started ? Should we here instead\n> > > reset all flips to their default values ? Using the newly introduced\n> > > setTrasform(Identity) wouldn't be enough as it would not clean up any\n> > > externally set flip...\n> > >\n> > > What would the expected configuration from the application point of\n> > > view ? Whatever flip is externally set is kept, or all flips\n> > > are reset to their default ? Please note that \"reset to their\n> > > defaults\" != \"clear any flip flag\" as most drivers (and we discussed\n> > > this with Dave recently) do inizialize their flip controls to report\n> > > what is actually programmed in registers, and quite often sensors are\n> > > 180 degrees flipped by default:\n> > > https://lore.kernel.org/all/CAPY8ntCCpMZtG-eJQ8cgd_GO06ZDP32oRK8mGrZKGMU2W401Dg@mail.gmail.com/\n> > > This is one more reason to remove the \"set flips to 0\" in init.\n> > >\n> > > Do we need a V4L2Device::resetFlips() function instead ?\n> >\n> > That's all interesting. Indeed I see there's a problem with devices\n> > that might have non-zero defaults, or where someone else has changed\n> > them without us being aware.\n> >\n> > As regards uvcvideo, maybe the best thing is then just to *do\n> > nothing*? At least that preserves the current behaviour. Although the\n> > pipeline handler doesn't support flips, someone could actually set\n> > them externally and they would be preserved. If the pipeline handler\n> > ever does support flips (should it one day?) we can figure out how it\n>\n> On \"should it be one day\": I presume so, but I can't tell if there are\n> uvc-specific reasons why this can't be made\n>\n> > might work then.\n>\n> I presume so. Again not a uvc expert\n>\n> >\n> > Probably the same argument holds for the simple PH too?\n> >\n> > I wonder if rkisp1 is different. That uses the CameraSensor class, so\n> > previously I assume it was clearing the flip bits. Therefore resetting\n> > the transform (it's always the Identity) might be correct? If a change\n>\n> It seems to me Simple uses CameraSensor as well, but yes, it's mostly\n> intended to work with YUV/RGB sensors. Nothing prevents to use RAW\n> sensors though. The idea is to have one day a GPU-based ISP\n> implementation that could be plugged to Simple, so RAW+Simple is a use\n> case we care about..\n\nPerhaps we should clear the flips in the simple PH too, if that's what\nit was doing previously. It might be right or wrong, but it's not any\nmore broken...!\n\n>\n> My concern is only about RAW sensors.\n>\n> - Assume a sensor driver reports BGGR with both v4l2 flips set to 1 and both\n> report V4L2_CTRL_FLAG_MODIFY_LAYOUT.\n> - With your patch we transform it to RGGB and report it as\n> properties::ColorFilterArrangement (should we rename it btw?).\n\nI don't mind ColorFilterArrangement... apart from the non-British\nspelling of \"Color\" of course!\n\n> - If any other user changes on filp it changes the bayer components\n> ordering too and applications might get confused ?\n>\n> Is something we should be concerned ?\n\nI think we should be concerned, yes. But perhaps for the moment we\nmight be better off just preserving the previous behaviour and at some\npoint someone will have to look again at them, perhaps when they have\nraw test cases.\n\nI'm feeling then that both rkisp1 and simple PHs should (possibly?\nperhaps? maybe?) clear the flips to be like they were before, and uvc\nshould leave them alone (again, like before). Does that sound like\nenough for the next version...?\n\nThanks!\nDavid\n\n>\n> > of behaviour is necessary (actually handling the transforms would be\n> > ideal!), that's probably a separate piece of work.\n> >\n> > Thoughts?\n> >\n> > David\n> >\n> > >\n> > > > V4L2DeviceFormat format;\n> > > > format.fourcc = data->video_->toV4L2PixelFormat(cfg.pixelFormat);\n> > > > format.size = cfg.size;\n> > > > --\n> > > > 2.30.2\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 4FDF2BD16B\n\tfor ;\n\tFri, 18 Nov 2022 17:27:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 889B36309B;\n\tFri, 18 Nov 2022 18:27:10 +0100 (CET)","from mail-pf1-x42e.google.com (mail-pf1-x42e.google.com\n\t[IPv6:2607:f8b0:4864:20::42e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D0E676308D\n\tfor ;\n\tFri, 18 Nov 2022 18:27:08 +0100 (CET)","by mail-pf1-x42e.google.com with SMTP id k22so5505336pfd.3\n\tfor ;\n\tFri, 18 Nov 2022 09:27:08 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668792430;\n\tbh=MEk5JhAHwttSvnrGtHOG7L8kdEHCO+6KdxmmR/6m5B4=;\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=oWHrPNKsglJAxz4tRLCT4taNJ7wjB/CDeDzI9X9ms1ksoT8PMHdgk4DhbR07oyHZL\n\tcSj2kT8sbBozIn7Lc7B8+QJKXVvWNfpufUzr5fABC/8qc0GA3kOuw8+WTnXlAFLk+n\n\tJeWA3xjYdTkc/5uKxyOvoDiotefD7PkFDUXgMfteET2n6ckljGfMdsuxRV0IhZO1rc\n\tSIlkVBYkMDNbw/Pe0x3YYZ5TRwm1Pm4f8rNeQlM58nKYIYBdYpbfmiNGbNvoO0vKME\n\tBDcNCOd/HePZzLp0YD7qp3GHyOz29gPEv/wMMiQQ4bwfKlh72iMgqwF7rgP6sUgb+h\n\tlsaAkKirtaCgg==","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=nPI4vSSpYFnHqih803rWohzlt8WX6YCIxBRIbSwpX9M=;\n\tb=qvpx7ttSYm0EEadr0YPurjZs0O3BFFoU+mNWpBYQz7Dqbp/O/1YFXqOc4PUy83f2Ky\n\t1iEoh2uO64j/yXkA2onpZ6se5qr4BlciPOQSrMlqEu/GuZITwFB+TQunJpp6uH8kQtSu\n\tFCPgBl2+JvjMSVzJWmHa2WU5Na5BMkKBsdpiUSUj5SxWN9VCu+h8Xi2wTYl/PNzq/DWJ\n\tNYY+Zi3RAVWVCY1MEBxxj1m5ODHMeM94q65lRH/a59LdvtmxeYOLk9zVFP8nrVRYl0c5\n\tcFMlHqv7+Og1mgo6C5RqtpUB7xw5zC1ylhg/+ZRraNF4IBSAwmixIcdyKMUZQG7agQVH\n\tP+PA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"qvpx7ttS\"; 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=nPI4vSSpYFnHqih803rWohzlt8WX6YCIxBRIbSwpX9M=;\n\tb=0yEPlpVi1KXDL5NRgtnhfgqG34prGG8HiPdNMidAMuuz9Hek52MFPIFaRKh6wapzYe\n\t4o0rYqfza8wzlr969MlSHIOTqvdP5hnrCz10cEuetvw7f8CjMW+LoC/PEwqH/seyPevf\n\tdUv2BY24/gVp2Hr5f5ovq4sqfstai2oLvniFu68igjxNvPfKtnGOXkt3ZG3U2cRK7Kaq\n\tFlp03Qszk/daGczW6dpMfas6P+ZiMYTvZ0SwI0Msm5wItOB88T2J6tR0D/KW4wbWD7l7\n\tOyUWDgXHRRvSNG5jDjtdZihlE9w0N/LAHmGfam/hbbtIEsF1kCfAe1pucYnb/r//B20w\n\tGWxQ==","X-Gm-Message-State":"ANoB5pnBLKGiQV9KRurKorcu5PX2tCjuZZd5sbSN9U1OilIZSrOiTFCO\n\tva65sTv1/rgGKLXnm8c6eNDk1iLkcr0NwhOMrMMqKA==","X-Google-Smtp-Source":"AA0mqf4VeN2hEghgQgSB4BeEjsiXY534vbK5IV1Wk+bZ+mDpzeE5uItUWtnStSMOvY5p/hj1MhsA0gALrP31kv52LkI=","X-Received":"by 2002:a63:c63:0:b0:476:c4e1:958b with SMTP id\n\t35-20020a630c63000000b00476c4e1958bmr7531699pgm.256.1668792427061;\n\tFri, 18 Nov 2022 09:27:07 -0800 (PST)","MIME-Version":"1.0","References":"<20221110144556.7858-1-david.plowman@raspberrypi.com>\n\t<20221110144556.7858-5-david.plowman@raspberrypi.com>\n\t<20221117103210.lum6d2ywokoimqxe@uno.localdomain>\n\t\n\t<20221117181702.cvgf3fxivlwiy4vu@uno.localdomain>","In-Reply-To":"<20221117181702.cvgf3fxivlwiy4vu@uno.localdomain>","Date":"Fri, 18 Nov 2022 17:26:56 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 4/6] libcamera: pipeline: uvcvideo:\n\tSet device's flip controls correctly","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,\n\tDave Stevenson ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]