[{"id":855,"web_url":"https://patchwork.libcamera.org/comment/855/","msgid":"<20190222104853.GE3522@pendragon.ideasonboard.com>","date":"2019-02-22T10:48:53","subject":"Re: [libcamera-devel] [PATCH 2/4] libcamera: pipeline: uvcvideo:\n\tenforce stream configuration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Thu, Feb 21, 2019 at 12:59:37AM +0100, Niklas Söderlund wrote:\n> The format requested by configureStreams() should exactly match what\n\ns/what$/what is/\n\n> programmed on the video device it self. If they do not match the call\n\ns/it self/itself/ or simply s/ it self//\n\n> should fail as the application in that case will not know what\n> configuration actually was programmed.\n\ns/actually was/was actually/\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/uvcvideo.cpp | 12 +++++++++++-\n> 1 file changed, 11 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 37a3477c2c56f3ea..b7b8ff109109e9c5 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -84,6 +84,7 @@ int PipelineHandlerUVC::configureStreams(Camera *camera,\n> \t\t\t\t\t std::map &config)\n> {\n> \tStreamConfiguration *cfg = &config[&stream_];\n> +\tint ret;\n> \n> \tLOG(UVC, Debug) << \"Configure the camera for resolution \"\n> \t\t\t<< cfg->width << \"x\" << cfg->height;\n> @@ -93,7 +94,16 @@ int PipelineHandlerUVC::configureStreams(Camera *camera,\n> \tformat.height = cfg->height;\n> \tformat.fourcc = cfg->pixelFormat;\n> \n> -\treturn video_->setFormat(&format);\n> +\tret = video_->setFormat(&format);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tif (format.width != cfg->width ||\n> +\t format.height != cfg->height ||\n> +\t format.fourcc != cfg->pixelFormat)\n> +\t\treturn -EINVAL;\n\nThis is fine for now, but I wonder if longer term we should enumerate\nthe available formats and resolutions, and avoiding calling setFormat()\nis the requested format can't be achieved. The documentation in patch\n1/4 explains that no configuration should be applied if an exact match\nis impossible, and calling setFormat() modifies the hardware\nconfiguration. On the other hand this change in hardware configuration\nisn't visible from the application, so it won't matter much.\n\n> +\n> +\treturn 0;\n> }\n> \n> int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A813600FD\n\tfor ;\n\tFri, 22 Feb 2019 11:48:58 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ECFDE255;\n\tFri, 22 Feb 2019 11:48:57 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1550832538;\n\tbh=xeZbvakg/QTd24cBUqttVzCMkSKDwSDmZwl0pA7Hxk8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VoAKnwcOhS2kRz4EKdS9s647lbIioVfICi73KIcdrSJoEK3PHlGxSdZPX7lW0yNxq\n\tWALZyX5E9zUaVXIMBeqrpvV7/RaQYrW3m8k3169AnWKtO9MQ2N3FkizgBfOKh/sGgf\n\tOnSV0m47tEaHOgpVOQ1+PckYrVtbNpjZ37RsewZM=","Date":"Fri, 22 Feb 2019 12:48:53 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190222104853.GE3522@pendragon.ideasonboard.com>","References":"<20190220235939.25147-1-niklas.soderlund@ragnatech.se>\n\t<20190220235939.25147-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190220235939.25147-3-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/4] libcamera: pipeline: uvcvideo:\n\tenforce stream configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 22 Feb 2019 10:48:58 -0000"}},{"id":866,"web_url":"https://patchwork.libcamera.org/comment/866/","msgid":"<20190223013901.GR11484@bigcity.dyn.berto.se>","date":"2019-02-23T01:39:01","subject":"Re: [libcamera-devel] [PATCH 2/4] libcamera: pipeline: uvcvideo:\n\tenforce stream configuration","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2019-02-22 12:48:53 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Thu, Feb 21, 2019 at 12:59:37AM +0100, Niklas Söderlund wrote:\n> > The format requested by configureStreams() should exactly match what\n> \n> s/what$/what is/\n> \n> > programmed on the video device it self. If they do not match the call\n> \n> s/it self/itself/ or simply s/ it self//\n> \n> > should fail as the application in that case will not know what\n> > configuration actually was programmed.\n> \n> s/actually was/was actually/\n\nI agree, will update this for 2/4 and 3/4 for next version.\n\n> \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/uvcvideo.cpp | 12 +++++++++++-\n> > 1 file changed, 11 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index 37a3477c2c56f3ea..b7b8ff109109e9c5 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -84,6 +84,7 @@ int PipelineHandlerUVC::configureStreams(Camera *camera,\n> > \t\t\t\t\t std::map &config)\n> > {\n> > \tStreamConfiguration *cfg = &config[&stream_];\n> > +\tint ret;\n> > \n> > \tLOG(UVC, Debug) << \"Configure the camera for resolution \"\n> > \t\t\t<< cfg->width << \"x\" << cfg->height;\n> > @@ -93,7 +94,16 @@ int PipelineHandlerUVC::configureStreams(Camera *camera,\n> > \tformat.height = cfg->height;\n> > \tformat.fourcc = cfg->pixelFormat;\n> > \n> > -\treturn video_->setFormat(&format);\n> > +\tret = video_->setFormat(&format);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tif (format.width != cfg->width ||\n> > +\t format.height != cfg->height ||\n> > +\t format.fourcc != cfg->pixelFormat)\n> > +\t\treturn -EINVAL;\n> \n> This is fine for now, but I wonder if longer term we should enumerate\n> the available formats and resolutions, and avoiding calling setFormat()\n> is the requested format can't be achieved. The documentation in patch\n> 1/4 explains that no configuration should be applied if an exact match\n> is impossible, and calling setFormat() modifies the hardware\n> configuration. On the other hand this change in hardware configuration\n> isn't visible from the application, so it won't matter much.\n\nI concur that that this is a short- to mid- term solution that allows us \nto move forward. Once we have a more complete StreamConfiguration \nimplementation and more pipeline handler implementations I think we \nmight be able to draw some more insights. At that point I think we will \nbe able to create one or more helper functions to assist pipeline \nhandlers, likely using the *_TRY_* IOCTLs. That way we could try a \ncomplete set of configurations in a none invasive fashion and fail in a \nmore safe way if needed.\n\nBonus for *_TRY_* IOCTLs support will be that applications would be able \nto call that themself before calling configureStreams() to 'update' and \ninspect the format they request to hardware/driver limitations before \nasking the hardware to be reprogrammed. At least this is how I model the \nfuture of this part of the library in my head, if anyone have other \nideas please let me know so I can try to have that in mind when working \non the interactions between the Camera and applications.\n\n> \n> > +\n> > +\treturn 0;\n> > }\n> > \n> > int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 43474600FB\n\tfor ;\n\tSat, 23 Feb 2019 02:39:04 +0100 (CET)","by mail-lf1-x141.google.com with SMTP id g12so3059893lfb.13\n\tfor ;\n\tFri, 22 Feb 2019 17:39:04 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tu9sm969467lfe.58.2019.02.22.17.39.01\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 22 Feb 2019 17:39:02 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=DWHmKV7nIDyn+ePfhqnAvOk0w7gqvXhUAp96uHIbvzw=;\n\tb=miceZDlWd3ZpAdnEW7FbhKbjZ3uyIoYWHKHLEl0pBDJVwT13ggWAG38cjRqpdzxAso\n\tMURybqyWrW3/VhqpoD1Ex/mn4HfYmI9iPGFzeUjWOrhDzigPheSHCPqnjKoOXpuLBODZ\n\tvKV0KxPE3QdsEdElHHi7ktvZWwYnOgK/jrLoVTdDB8VHpgIXBIA2TvS1rA9YkEczO8tA\n\txmJ89Wb+8p3FBnnNAqh8sFomYaFiN3zoC3Ha63x92hzBOgsCFvRbTUnSwfPwKWvfGCTH\n\tzadYZhKR5xQ9jg4BRkNkT3Muczd3lweZQaSTLsgpBaNMgUVTFoI2PGM+G3UlharTYInN\n\tbUew==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=DWHmKV7nIDyn+ePfhqnAvOk0w7gqvXhUAp96uHIbvzw=;\n\tb=Zi39rtQG8C0EBC2KkEVQBw93+r7lDwIwRm3fynYK3HgK8dLZ54HmeJ1opQ2kqKvKxv\n\tbdulalCOb8hRH1NAat5qmvtJnxiq/OczoS/86iB99gX8RwiY7g9EHqLXkuuGiVhlscUX\n\t55YZC7z/GzxN4tj6RAbbuh318if1kOytBAGf4ktDU+mAyknEJQKBk1Bt9jjbUQE949Il\n\teke2zDOpi2qByjQac8b6Re/x6ixr1RNUBaPT5heoEMI1375YlLv1XASBzjtjB6bruArX\n\tIIXujjAyfIPBJQpqD2EDml9OTry8iRSrbN7iCoxjRXaz7bBR0FLWNvQKPkTWvekcs/HG\n\tuhdg==","X-Gm-Message-State":"AHQUAuaTkdQGfgj+svhMX46ADcPWytSsJAM1DnLV0voNfmWBy+8It7sW\n\toZg3+38OYA+Q1DmF5LvFrmG2Cg==","X-Google-Smtp-Source":"AHgI3IaCsK72VR5S/2Etzag0p1oGW3UWidOJ/3yC9lvv5dWkAptGluL8KXI5/gPFXZsDxPWUSYP5/w==","X-Received":"by 2002:ac2:510e:: with SMTP id\n\tq14mr3728865lfb.106.1550885943264; \n\tFri, 22 Feb 2019 17:39:03 -0800 (PST)","Date":"Sat, 23 Feb 2019 02:39:01 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190223013901.GR11484@bigcity.dyn.berto.se>","References":"<20190220235939.25147-1-niklas.soderlund@ragnatech.se>\n\t<20190220235939.25147-3-niklas.soderlund@ragnatech.se>\n\t<20190222104853.GE3522@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190222104853.GE3522@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/4] libcamera: pipeline: uvcvideo:\n\tenforce stream configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 23 Feb 2019 01:39:04 -0000"}},{"id":870,"web_url":"https://patchwork.libcamera.org/comment/870/","msgid":"<20190225083910.GB4387@pendragon.ideasonboard.com>","date":"2019-02-25T08:39:10","subject":"Re: [libcamera-devel] [PATCH 2/4] libcamera: pipeline: uvcvideo:\n\tenforce stream configuration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Sat, Feb 23, 2019 at 02:39:01AM +0100, Niklas Söderlund wrote:\n> On 2019-02-22 12:48:53 +0200, Laurent Pinchart wrote:\n> > On Thu, Feb 21, 2019 at 12:59:37AM +0100, Niklas Söderlund wrote:\n> >> The format requested by configureStreams() should exactly match what\n> > \n> > s/what$/what is/\n> > \n> >> programmed on the video device it self. If they do not match the call\n> > \n> > s/it self/itself/ or simply s/ it self//\n> > \n> >> should fail as the application in that case will not know what\n> >> configuration actually was programmed.\n> > \n> > s/actually was/was actually/\n> \n> I agree, will update this for 2/4 and 3/4 for next version.\n> \n> >> Signed-off-by: Niklas Söderlund \n> >> ---\n> >> src/libcamera/pipeline/uvcvideo.cpp | 12 +++++++++++-\n> >> 1 file changed, 11 insertions(+), 1 deletion(-)\n> >> \n> >> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> >> index 37a3477c2c56f3ea..b7b8ff109109e9c5 100644\n> >> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> >> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> >> @@ -84,6 +84,7 @@ int PipelineHandlerUVC::configureStreams(Camera *camera,\n> >> \t\t\t\t\t std::map &config)\n> >> {\n> >> \tStreamConfiguration *cfg = &config[&stream_];\n> >> +\tint ret;\n> >> \n> >> \tLOG(UVC, Debug) << \"Configure the camera for resolution \"\n> >> \t\t\t<< cfg->width << \"x\" << cfg->height;\n> >> @@ -93,7 +94,16 @@ int PipelineHandlerUVC::configureStreams(Camera *camera,\n> >> \tformat.height = cfg->height;\n> >> \tformat.fourcc = cfg->pixelFormat;\n> >> \n> >> -\treturn video_->setFormat(&format);\n> >> +\tret = video_->setFormat(&format);\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tif (format.width != cfg->width ||\n> >> +\t format.height != cfg->height ||\n> >> +\t format.fourcc != cfg->pixelFormat)\n> >> +\t\treturn -EINVAL;\n> > \n> > This is fine for now, but I wonder if longer term we should enumerate\n> > the available formats and resolutions, and avoiding calling setFormat()\n> > is the requested format can't be achieved. The documentation in patch\n> > 1/4 explains that no configuration should be applied if an exact match\n> > is impossible, and calling setFormat() modifies the hardware\n> > configuration. On the other hand this change in hardware configuration\n> > isn't visible from the application, so it won't matter much.\n> \n> I concur that that this is a short- to mid- term solution that allows us \n> to move forward. Once we have a more complete StreamConfiguration \n> implementation and more pipeline handler implementations I think we \n> might be able to draw some more insights. At that point I think we will \n> be able to create one or more helper functions to assist pipeline \n> handlers, likely using the *_TRY_* IOCTLs. That way we could try a \n> complete set of configurations in a none invasive fashion and fail in a \n> more safe way if needed.\n\nThe TRY ioctls will usually not help much as they're limited to video\nnodes. Something more complex will be needed for MC-based pipelines, but\nwe can do that.\n\n> Bonus for *_TRY_* IOCTLs support will be that applications would be able \n> to call that themself before calling configureStreams() to 'update' and \n> inspect the format they request to hardware/driver limitations before \n> asking the hardware to be reprogrammed. At least this is how I model the \n> future of this part of the library in my head, if anyone have other \n> ideas please let me know so I can try to have that in mind when working \n> on the interactions between the Camera and applications.\n\nI'm not sure if that will be feasible. While this is the model exposed\nby V4L2, I fell it may not be the most suitable for applications. We'll\nhave to think about it.\n\n> >> +\n> >> +\treturn 0;\n> >> }\n> >> \n> >> int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 75EC6600FE\n\tfor ;\n\tMon, 25 Feb 2019 09:39:19 +0100 (CET)","from pendragon.ideasonboard.com (85-76-109-165-nat.elisa-mobile.fi\n\t[85.76.109.165])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7D4AF67;\n\tMon, 25 Feb 2019 09:39:18 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1551083959;\n\tbh=CCq8ZXIwABWQS0Sq67sqK71sFfxah1QOjJ6/8VD/CRs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IpdXqKI5+pDJn+SWjvhtDAwcBHbXkviSJRfikosGiLN7yY9ur/ZxQO7XdTOwtMmD1\n\tezDQyuUvohJUrtA7LuKJbsM4Sj5i1Sit9QmILah9gchAGNTTQis2j55goyG59PZPBm\n\tzpUjOIKxhmONtvFBdke1+2w56FwD7RbMykqBxPk0=","Date":"Mon, 25 Feb 2019 10:39:10 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190225083910.GB4387@pendragon.ideasonboard.com>","References":"<20190220235939.25147-1-niklas.soderlund@ragnatech.se>\n\t<20190220235939.25147-3-niklas.soderlund@ragnatech.se>\n\t<20190222104853.GE3522@pendragon.ideasonboard.com>\n\t<20190223013901.GR11484@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190223013901.GR11484@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 2/4] libcamera: pipeline: uvcvideo:\n\tenforce stream configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 25 Feb 2019 08:39:19 -0000"}}]