[{"id":31236,"web_url":"https://patchwork.libcamera.org/comment/31236/","msgid":"","date":"2024-09-16T07:35:04","subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:\n> Hi folks,\n>\n> Currently applications set resolutions, pixelFormat, bufferCount, etc,\n> into StreamConfigurations, and Pipeline Handler decides which streams\n> they're assigned to. However, it doesn't allow application to assign\n> streams that cannot be distinguished by those arguments into\n> VideoRecording or StillCapture (say YUV/NV12 format), which is needed in\n> mtkisp7.\n\nCould you explain in a bit more detail why this \"is needed\" and how\nyou plan to use StreamRole as part of the stream configuration ?\n\n>\n> This patch allows application to set the desired StreamRole directly.\n>\n> This patch passed gitlab pipeline:\n> https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1271770\n>\n> It might be controversial. Let me know the concerns. Thanks!\n>\n\nThe fact is that roles are designed to hint libcamera about how to\npopulate the CameraConfiguration, after that point they have no\nmeaning anymore for the library.\n\nI presum you need this for Android, right ? Isn't it better to keep\nthe association between the Android role and the libcamera Stream in\nthe Android HAL instead ? Be aware that the CameraConfiguration should\nbe populated with StreamConfigurations in the same order as the\nStreamRole order the application asked for.\n\nIf instead we want to keep the Role in Stream, it shouldn't be up to\nthe application to populate it, but it should be the pipeline's\ngenerateConfiguration() function that should do this probably.\n\nThanks\n j\n\n> BR,\n> Harvey\n>\n>\n>\n> Han-Lin Chen (1):\n> libcamera: Add StreamRole into StreamConfiguration\n>\n> include/libcamera/stream.h | 2 ++\n> src/libcamera/stream.cpp | 12 ++++++++++--\n> 2 files changed, 12 insertions(+), 2 deletions(-)\n>\n> --\n> 2.46.0.662.g92d0881bb0-goog\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 D3449C3257\n\tfor ;\n\tMon, 16 Sep 2024 07:35:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 02511634FC;\n\tMon, 16 Sep 2024 09:35:09 +0200 (CEST)","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 08AB6618F0\n\tfor ;\n\tMon, 16 Sep 2024 09:35:08 +0200 (CEST)","from ideasonboard.com (unknown [193.34.101.21])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9F2AF497;\n\tMon, 16 Sep 2024 09:33:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZKPICpLB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726472026;\n\tbh=AOxQ6jBWSDEtBip6oA5masMADWEUcbEWG/fBFaC0gB8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZKPICpLB2zHUKp4QI9ZbrXmTVBsY1wAekwBiTANjWB3pIA1frbYcAi3sT81KSWjpb\n\tu7FNQrGEGkDKr3IWqsuX8EAxYuhCbuR1o0ITbonr7/XSk/HYIY3vAsPfMAsJw8WTN/\n\tALeS7clpkkSHcIs4R3iydbj7AUGfGhNWXDYUH3nY=","Date":"Mon, 16 Sep 2024 09:35:04 +0200","From":"Jacopo Mondi ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHarvey Yang ","Subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","Message-ID":"","References":"<20240916045802.3799103-1-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240916045802.3799103-1-chenghaoyang@google.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31237,"web_url":"https://patchwork.libcamera.org/comment/31237/","msgid":"","date":"2024-09-16T07:51:00","subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nOn Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi \nwrote:\n\n> Hi Harvey\n>\n> On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:\n> > Hi folks,\n> >\n> > Currently applications set resolutions, pixelFormat, bufferCount, etc,\n> > into StreamConfigurations, and Pipeline Handler decides which streams\n> > they're assigned to. However, it doesn't allow application to assign\n> > streams that cannot be distinguished by those arguments into\n> > VideoRecording or StillCapture (say YUV/NV12 format), which is needed in\n> > mtkisp7.\n>\n> Could you explain in a bit more detail why this \"is needed\" and how\n> you plan to use StreamRole as part of the stream configuration ?\n>\n> >\n> > This patch allows application to set the desired StreamRole directly.\n> >\n> > This patch passed gitlab pipeline:\n> >\n> https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1271770\n> >\n> > It might be controversial. Let me know the concerns. Thanks!\n> >\n>\n> The fact is that roles are designed to hint libcamera about how to\n> populate the CameraConfiguration, after that point they have no\n> meaning anymore for the library.\n>\n> I presum you need this for Android, right ?\n\n\nCorrect :)\n\n\n> Isn't it better to keep\n> the association between the Android role and the libcamera Stream in\n> the Android HAL instead ?\n\n\nSorry, I don't quite get your point. What's the Android role?\nWhen doing `validate()` or `configure()`, how does Android HAL/adapter let\nPipeline Handler know which StreamConfiguration the HAL desires it to\nbelong to?\n\n\n> Be aware that the CameraConfiguration should\n> be populated with StreamConfigurations in the same order as the\n> StreamRole order the application asked for.\n>\n\nYes, I remember that.\n\n\n>\n> If instead we want to keep the Role in Stream, it shouldn't be up to\n> the application to populate it, but it should be the pipeline's\n> generateConfiguration() function that should do this probably.\n>\n>\nYes, actually in mtkisp7's implementation, that's how we do it [1].\nBasically Android HAL/adapter will not change the StreamRole in\nStreamConfiguration, while IIUC the API is designed in the way that\napplications can change any attribute in a StreamConfiguration,\nbefore calling `validate()` or `configuration()`, and\n`Camera::generateConfiguration()` only provides the initial/suggested\nconfiguration, right?\n\n[1]:\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=563\n\nBR,\nHarvey\n\nThanks\n> j\n>\n> > BR,\n> > Harvey\n> >\n> >\n> >\n> > Han-Lin Chen (1):\n> > libcamera: Add StreamRole into StreamConfiguration\n> >\n> > include/libcamera/stream.h | 2 ++\n> > src/libcamera/stream.cpp | 12 ++++++++++--\n> > 2 files changed, 12 insertions(+), 2 deletions(-)\n> >\n> > --\n> > 2.46.0.662.g92d0881bb0-goog\n> >\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 C8F21C324C\n\tfor ;\n\tMon, 16 Sep 2024 07:51:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE69F634F5;\n\tMon, 16 Sep 2024 09:51:15 +0200 (CEST)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 87F6F618F0\n\tfor ;\n\tMon, 16 Sep 2024 09:51:13 +0200 (CEST)","by mail-lj1-x234.google.com with SMTP id\n\t38308e7fff4ca-2f75de9a503so42506951fa.0\n\tfor ;\n\tMon, 16 Sep 2024 00:51:13 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"DG8wZS52\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1726473072; x=1727077872;\n\tdarn=lists.libcamera.org; \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=wPlBWZ3za00Fzb7pbgkvjCdVnb1bDBwMd3YIq/XeKJg=;\n\tb=DG8wZS52cxPNjiXDBP8uWyDFE/fDql7HTel9I6YUzhhjhPRbLFeS/o3q1Voqdi30PB\n\tJ+Y8RFdCdHCraVd0ocVSmU27VqbmXMmcXIsypd7/H9ajetAPY4z+K4uOHAhHf3oTJEBI\n\tmPG1oHFKs1SkWd7/cKBH5K5qaKFsCJgtnpF88=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1726473072; x=1727077872;\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=wPlBWZ3za00Fzb7pbgkvjCdVnb1bDBwMd3YIq/XeKJg=;\n\tb=aJXfzyYKDlCjC5wD6hEkBjDiIFW0WOliUHdh16vtLUiOloWmdOHDZBchqFuzmFu+bK\n\tNh9L9rj3XEPe0bc5pLdNRDHbjfR2ueYiexZ8KjQa2qpU5AaFfXd7wn2z3UlEtwqoFUMH\n\ty+rOjtFK+KudQzXDuj/tJ1FNgPNjbQo/oJ5zqSlhwd8tUg46raHipVsMKbHdbBu4p6y9\n\t7KoIt2wcdmSjLVC3Q/AZfdizY+p0j0Bt34W9KzJi9b5h7FaHorpYgvSA5ln7CO5V/zyj\n\tEAskXBKPb731I5n80Y0KDZCVKM/ARnUz0hpsSAu4v78QtPVEnUSIZCBaCaoHxgdKdCzc\n\tKNSg==","X-Gm-Message-State":"AOJu0YxIUDVMuilhCOg0AQJIfSa9+L0KpZd2gbcZITIZKqDOgPzN74LV\n\tChLQs3ugZNb0v1kivTzeS4ZHNEveVt81VPb2wt27qB8dLRaeYqm9Jn+Ss59HKwitM6yBORxlo40\n\tuIhQ2UcvrU6EznpGay7Q/DyrUy9IN0jwP2D2pUeEw+LOlPko1HA==","X-Google-Smtp-Source":"AGHT+IHKZfga9gL/kTkC33XI58/5kULUXj73bXUKYuAYeZDj+t+m82Eos9eXsziCnwmgwcTAUpsJaza12WbzgIKXXEc=","X-Received":"by 2002:a2e:515a:0:b0:2f3:b71a:1e91 with SMTP id\n\t38308e7fff4ca-2f787dc4303mr53022981fa.17.1726473072074;\n\tMon, 16 Sep 2024 00:51:12 -0700 (PDT)","MIME-Version":"1.0","References":"<20240916045802.3799103-1-chenghaoyang@google.com>\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Mon, 16 Sep 2024 15:51:00 +0800","Message-ID":"","Subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHarvey Yang ","Content-Type":"multipart/alternative; boundary=\"0000000000002d05fc062237d60a\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31242,"web_url":"https://patchwork.libcamera.org/comment/31242/","msgid":"","date":"2024-09-16T12:45:21","subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:\n> Hi Jacopo,\n>\n> On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi \n> wrote:\n>\n> > Hi Harvey\n> >\n> > On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:\n> > > Hi folks,\n> > >\n> > > Currently applications set resolutions, pixelFormat, bufferCount, etc,\n> > > into StreamConfigurations, and Pipeline Handler decides which streams\n> > > they're assigned to. However, it doesn't allow application to assign\n> > > streams that cannot be distinguished by those arguments into\n> > > VideoRecording or StillCapture (say YUV/NV12 format), which is needed in\n> > > mtkisp7.\n> >\n> > Could you explain in a bit more detail why this \"is needed\" and how\n> > you plan to use StreamRole as part of the stream configuration ?\n> >\n\nCould you explain in a bit more detail why this \"is needed\" and how\nyou plan to use StreamRole as part of the stream configuration ?","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 4996BC324C\n\tfor ;\n\tMon, 16 Sep 2024 12:45:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 222D8634F8;\n\tMon, 16 Sep 2024 14:45:27 +0200 (CEST)","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 52CC4634F5\n\tfor ;\n\tMon, 16 Sep 2024 14:45:25 +0200 (CEST)","from ideasonboard.com (unknown [193.34.101.21])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AC16682A;\n\tMon, 16 Sep 2024 14:44:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FuQS84k+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726490643;\n\tbh=oaleO99uHHAg3Vqu3aJfwBBquQhuSkpP27XGniRZ/Rw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FuQS84k+c3BrMO2gR2D1n0Ut59ds3RZ58WcnE4AW5ksRGHo/8FwkCXJtEeEhC9nW+\n\tLa8I83nZg++24AA+Pp2jNIcpq4yQKzbcBPDJEMKfBtX+WC2vgq8zFqiawDO922x+ke\n\tgaThj9sy+DnYm8U1JubKuN5oHJBEWGcA0VsASUsQ=","Date":"Mon, 16 Sep 2024 14:45:21 +0200","From":"Jacopo Mondi ","To":"Cheng-Hao Yang ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tHarvey Yang ","Subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","Message-ID":"","References":"<20240916045802.3799103-1-chenghaoyang@google.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31260,"web_url":"https://patchwork.libcamera.org/comment/31260/","msgid":"","date":"2024-09-18T08:33:27","subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Sorry Jacopo, my bad to miss the first message:\n\nOn Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi \nwrote:\n\n> Hi Harvey\n>\n> On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:\n> > Hi Jacopo,\n> >\n> > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi <\n> jacopo.mondi@ideasonboard.com>\n> > wrote:\n> >\n> > > Hi Harvey\n> > >\n> > > On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:\n> > > > Hi folks,\n> > > >\n> > > > Currently applications set resolutions, pixelFormat, bufferCount,\n> etc,\n> > > > into StreamConfigurations, and Pipeline Handler decides which streams\n> > > > they're assigned to. However, it doesn't allow application to assign\n> > > > streams that cannot be distinguished by those arguments into\n> > > > VideoRecording or StillCapture (say YUV/NV12 format), which is\n> needed in\n> > > > mtkisp7.\n> > >\n> > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > you plan to use StreamRole as part of the stream configuration ?\n> > >\n>\n> Could you explain in a bit more detail why this \"is needed\" and how\n> you plan to use StreamRole as part of the stream configuration ?\n>\n>\n>\nIn mtkisp7 pipeline handler, both preview (or video) and still capture\nstreams support the same format (NV12) and bufferCount, and the\nmaximum resolutions are also the same. Therefore, when calling\n`validate()` and `configure()`, mtkisp7 pipeline handler doesn't know\nif applications/Android adapter wants the stream to go through the\nstill captur pipeline or not.\n\nDoes that make sense :P?\n\nBR,\nHarvey","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 53BB8C3257\n\tfor ;\n\tWed, 18 Sep 2024 08:33:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F13DF634F9;\n\tWed, 18 Sep 2024 10:33:41 +0200 (CEST)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D34C2618E0\n\tfor ;\n\tWed, 18 Sep 2024 10:33:39 +0200 (CEST)","by mail-lj1-x230.google.com with SMTP id\n\t38308e7fff4ca-2f759688444so56100111fa.1\n\tfor ;\n\tWed, 18 Sep 2024 01:33:39 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"JoNOdsML\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1726648419; x=1727253219;\n\tdarn=lists.libcamera.org; \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=m1HLT5E4wmNoKrquyacLP96NKdJ198ZpsYgIqtaZFhI=;\n\tb=JoNOdsMLk+TQ7ukT3kRS8st6wRuqwb8d/jU9JScgXwbnLcY4zD2w7LrLz+AY+zIdqN\n\ttHtLAAZXxvg8mP0LBgKMlsGDpogLnMX6gV5jUOES8GGG04UK2MK3oBLH0ob12G7HoW+M\n\truW9WISRK7Kfw/+N6V0/eQWL7Gboo2aOupOBw=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1726648419; x=1727253219;\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=m1HLT5E4wmNoKrquyacLP96NKdJ198ZpsYgIqtaZFhI=;\n\tb=FlhfTGSwDwZ5nh0x2ccU1Ycffy2QAFiEYYyfKwNm3UaTnSjArNze5bUlotFj2XRSbU\n\tpu9G84yaWz53qnFaJ506PyHUkcc4QBtrH9EkPfamx8e9Jlb6eYCdP6GNUr7fYdWM7pBA\n\tQyZH24tAkCpb0R0C8T7FqX1HJF0IvJXflj1Ftj/gcdw4oe7I54EsSdgKAyJorlYEbj91\n\tNTO7OU1BnwFSqN/YRVSjxMT30IZI657AyU9jw/NEWdi9WbNw1mKovpFcf0wOJcZJmCyJ\n\tift5drtdX8KctPy7STUIyZoMhC7s21mJg4cy6VR9ppzEJpKPBcKdZfCJljCnKBIvjjtQ\n\tKyPg==","X-Gm-Message-State":"AOJu0YzygxrP9RXqgAZsh5ft6ta6ohQdDko4cThtrad8gCTOWDTOELCy\n\tzpf5fyIvLBWDU7f3x2lZVa2jkeUNSOXG4c0cPOEbJ0z1vHvL+vLGn/MRyHtDFxqdvR+Vh92gK6k\n\toiOkOBnRYHBkBQJVcv94tPt49PrtK/6ccgGPxs5RdglLvGzI=","X-Google-Smtp-Source":"AGHT+IHzAMWUMB4XYNPiJTB1NAvAqHgqEd3jcgWc2WA0AM+8w2Qa64bXjs2fMeX9oSdstnNVQvBIXvSvaTR7RTNlkRw=","X-Received":"by 2002:a2e:be25:0:b0:2f7:7b34:285f with SMTP id\n\t38308e7fff4ca-2f787ed8881mr106954501fa.20.1726648418763;\n\tWed, 18 Sep 2024 01:33:38 -0700 (PDT)","MIME-Version":"1.0","References":"<20240916045802.3799103-1-chenghaoyang@google.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Wed, 18 Sep 2024 16:33:27 +0800","Message-ID":"","Subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHarvey Yang ","Content-Type":"multipart/alternative; boundary=\"000000000000a723c4062260a90e\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31270,"web_url":"https://patchwork.libcamera.org/comment/31270/","msgid":"","date":"2024-09-19T09:09:34","subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Wed, Sep 18, 2024 at 04:33:27PM GMT, Cheng-Hao Yang wrote:\n> Sorry Jacopo, my bad to miss the first message:\n>\n> On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi \n> wrote:\n>\n> > Hi Harvey\n> >\n> > On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:\n> > > Hi Jacopo,\n> > >\n> > > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi <\n> > jacopo.mondi@ideasonboard.com>\n> > > wrote:\n> > >\n> > > > Hi Harvey\n> > > >\n> > > > On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:\n> > > > > Hi folks,\n> > > > >\n> > > > > Currently applications set resolutions, pixelFormat, bufferCount,\n> > etc,\n> > > > > into StreamConfigurations, and Pipeline Handler decides which streams\n> > > > > they're assigned to. However, it doesn't allow application to assign\n> > > > > streams that cannot be distinguished by those arguments into\n> > > > > VideoRecording or StillCapture (say YUV/NV12 format), which is\n> > needed in\n> > > > > mtkisp7.\n> > > >\n> > > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > > you plan to use StreamRole as part of the stream configuration ?\n> > > >\n> >\n> > Could you explain in a bit more detail why this \"is needed\" and how\n> > you plan to use StreamRole as part of the stream configuration ?\n> >\n> >\n> >\n> In mtkisp7 pipeline handler, both preview (or video) and still capture\n> streams support the same format (NV12) and bufferCount, and the\n> maximum resolutions are also the same. Therefore, when calling\n> `validate()` and `configure()`, mtkisp7 pipeline handler doesn't know\n> if applications/Android adapter wants the stream to go through the\n> still captur pipeline or not.\n\nI still have an hard time understanding why validate() and configure()\nneeds to know the \"role\". Is this to assign \"pipes\" to Streams ? Does\nthe mtkisp7 pipes have different capabilities when it comes to\nsupported formats and resolutions ?\n\n>\n> Does that make sense :P?\n\nI guess so, but I wonder if the \"role\" is the main criteria that\nshould be taken into account when assiging pipes to streams.\n\nHow would this work for applications that operates on platforms where\nthe pipe selection depends on other criteria like the supported image\nformats and stream resolutions ? In example, for rkisp1\nthe \"self\" pipelines can do RGB while the \"main\" can't and that's how\nwe assign pipes during validate().\n\nTo be honest this has not proven to be optimal and I'm not opposed to\nadd Roles to the pipe selection criteria, but they shouldn't be made the\nonly criteria on which pipes are assigned (unless we support this on\nall pipelines).\n\nAlso I would not based any future-proof design on Roles, they will\nlikely be heavily reworked or removed going forward.\n\nAs your main use case is Android, I think it would be doable for you\nto\n1) Request a generateConfiguration() and keep track of the\n StreamConfiguration order.\n\n generateConfiguration(Viewfinder, Still, Raw)\n\n will give you StreamConfiguration for the above rols in order as\n you have requested them, if supported.\n\n2) Code validate() so that you try to assing pipes based on the\n formats/sizes and if formats/sizes are the same define an ordering.\n In example the first YUV/WxH stream goes to Viewfinder if an\n identical YUV/WxH stream is requested for Still Capture.\n\n3) As validate() assigns Steams * to StreamConfiguration (with\n ::setStream) it should be easy to keep track to which pipe a\n StreamConfiguration has been assigned to at configure() time.\n\nCould you list what are the platform's pipes capabilities ?\n\nAs said, I'm not opposed to use Roles for assigning pipes, but I don't\nthink we should based any new development on Roles as there's an high\nchance they can be reworked.\n\nCc-Laurent for opinions.\n\n>\n> BR,\n> Harvey","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 A43ACC324C\n\tfor ;\n\tThu, 19 Sep 2024 09:09:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 91235634FC;\n\tThu, 19 Sep 2024 11:09:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6B935618E4\n\tfor ;\n\tThu, 19 Sep 2024 11:09:38 +0200 (CEST)","from ideasonboard.com (unknown [83.68.141.146])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C64B5236;\n\tThu, 19 Sep 2024 11:08:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YQYwMYw1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726736894;\n\tbh=6gXXq1734WwY8SEtnqHa40jxBjfo4beBJB5/0fv6Uwo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YQYwMYw1op1RBqosK/1zMHfuGWJ/CHn3iHn73WGS96hqf34zKT6ilwuX0fJXFFPly\n\tWrJ1nNTVmhDJDgFlSJjNFZDa7h1PFdqV3XWfjtO+ajnSWJJNPPq60YJTbyC1CCW9mN\n\tgnZwSXPVEDVJNO8qh4htY2+HFYVeFfDobg4U4yJs=","Date":"Thu, 19 Sep 2024 11:09:34 +0200","From":"Jacopo Mondi ","To":"Cheng-Hao Yang ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tHarvey Yang , \n\tLaurent Pinchart ","Subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","Message-ID":"","References":"<20240916045802.3799103-1-chenghaoyang@google.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31273,"web_url":"https://patchwork.libcamera.org/comment/31273/","msgid":"","date":"2024-09-19T13:55:58","subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","submitter":{"id":148,"url":"https://patchwork.libcamera.org/api/people/148/","name":"Cheng-Hao Yang","email":"chenghaoyang@google.com"},"content":"Hi Jacopo and Laurent,\n\nOn Thu, Sep 19, 2024 at 5:09 PM Jacopo Mondi \nwrote:\n\n> Hi Harvey\n>\n> On Wed, Sep 18, 2024 at 04:33:27PM GMT, Cheng-Hao Yang wrote:\n> > Sorry Jacopo, my bad to miss the first message:\n> >\n> > On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi <\n> jacopo.mondi@ideasonboard.com>\n> > wrote:\n> >\n> > > Hi Harvey\n> > >\n> > > On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi <\n> > > jacopo.mondi@ideasonboard.com>\n> > > > wrote:\n> > > >\n> > > > > Hi Harvey\n> > > > >\n> > > > > On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:\n> > > > > > Hi folks,\n> > > > > >\n> > > > > > Currently applications set resolutions, pixelFormat, bufferCount,\n> > > etc,\n> > > > > > into StreamConfigurations, and Pipeline Handler decides which\n> streams\n> > > > > > they're assigned to. However, it doesn't allow application to\n> assign\n> > > > > > streams that cannot be distinguished by those arguments into\n> > > > > > VideoRecording or StillCapture (say YUV/NV12 format), which is\n> > > needed in\n> > > > > > mtkisp7.\n> > > > >\n> > > > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > > > you plan to use StreamRole as part of the stream configuration ?\n> > > > >\n> > >\n> > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > you plan to use StreamRole as part of the stream configuration ?\n> > >\n> > >\n> > >\n> > In mtkisp7 pipeline handler, both preview (or video) and still capture\n> > streams support the same format (NV12) and bufferCount, and the\n> > maximum resolutions are also the same. Therefore, when calling\n> > `validate()` and `configure()`, mtkisp7 pipeline handler doesn't know\n> > if applications/Android adapter wants the stream to go through the\n> > still captur pipeline or not.\n>\n> I still have an hard time understanding why validate() and configure()\n> needs to know the \"role\". Is this to assign \"pipes\" to Streams ?\n\n\nIf I understand correctly that \"pipes\" means the pipelines of ISP/IPA algos\nfor preview/video/still capture respectively, yes, it's to assign the\nStreamConfiguration(s) to the desired pipe(s), which means to call\n`StreamConfiguration::setStream()`. According to the comments [1], it's\nsupposed to be called in `PipelineHandler::configure()`, like\nSimplePipelineHandler [2]. However, there are also some pipeline handlers\nthat set them in `CameraConfiguration::validate()` [3] [4].\n\n[1]:\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n303\n[2]:\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/simple/simple.cpp#n1270\n[3]:\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n347\n[4]:\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rkisp1/rkisp1.cpp#n520\n\nDoes\n> the mtkisp7 pipes have different capabilities when it comes to\n> supported formats and resolutions ?\n>\n\nNo, as you can see in mtkisp7's implementation [5], all of them support the\nsame formats and resolutions.\n\n[5]:\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n\n\n\n\n> >\n> > Does that make sense :P?\n>\n> I guess so, but I wonder if the \"role\" is the main criteria that\n> should be taken into account when assiging pipes to streams.\n>\nHow would this work for applications that operates on platforms where\n> the pipe selection depends on other criteria like the supported image\n> formats and stream resolutions ? In example, for rkisp1\n> the \"self\" pipelines can do RGB while the \"main\" can't and that's how\n> we assign pipes during validate().\n\n\n> To be honest this has not proven to be optimal and I'm not opposed to\n> add Roles to the pipe selection criteria, but they shouldn't be made the\n> only criteria on which pipes are assigned (unless we support this on\n> all pipelines).\n>\n>\nI agree that the `role` might not be the main criteria in general cases.\nIt's just that we find difficulties in assigning them properly with the\ncurrent\nconfigurations in mtkisp7.\n\n\n\n> Also I would not based any future-proof design on Roles, they will\n> likely be heavily reworked or removed going forward.\n>\n> As your main use case is Android, I think it would be doable for you\n> to\n> 1) Request a generateConfiguration() and keep track of the\n> StreamConfiguration order.\n>\n> generateConfiguration(Viewfinder, Still, Raw)\n>\n> will give you StreamConfiguration for the above rols in order as\n> you have requested them, if supported.\n>\n> 2) Code validate() so that you try to assing pipes based on the\n> formats/sizes and if formats/sizes are the same define an ordering.\n> In example the first YUV/WxH stream goes to Viewfinder if an\n> identical YUV/WxH stream is requested for Still Capture.\n>\n\nThis actually assumes the application doesn't change the content of the\ndefault CameraConfiguration/StreamConfiguration, which I doubt if it's a\ngood thing. Currently in Android adapter, it re-arranges [6]\nStreamConfigurations based on the previously-fetched default\nStreamConfigurations of the StreamRoles, which I think is a normal\npractice of the current interfaces' logic. Please correct me if I'm wrong :)\n\n[6]:\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n708\n\nAlso, if an application calls\n`generateConfiguration(camera, {StreamRole::VideoRecording})` and\n`generateConfiguration(camera, {StreamRole::StillCapture})`\nrespectively, and ends up calling `configure()` with `VideoRecording`'s\nCameraConfiguration, how would the pipeline handler know that the\napplication intends to get buffers from the stream `VideoRecording`?\nIn your suggestion, it seems that the application needs to call\n`generateConfiguration(camera, {StreamRole::VideoRecording})` again,\nwhich I don't think is enforced in the current libcamera API, and might\nnot be a good practice.\n\n\n> 3) As validate() assigns Steams * to StreamConfiguration (with\n> ::setStream) it should be easy to keep track to which pipe a\n> StreamConfiguration has been assigned to at configure() time.\n>\n> Could you list what are the platform's pipes capabilities ?\n>\n>\nFor example, on `ciri` with mtkisp7 pipeline handler [5], `pixelFormat` is\nNV12,\nsupported resolution range is `320x240` to `2592x1944`, and `bufferCount` is\n8. It applies to `StillCapture`, `VideoRecording`, and `ViewFinder`.\n\n[5]:\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n\nAs said, I'm not opposed to use Roles for assigning pipes, but I don't\n> think we should based any new development on Roles as there's an high\n> chance they can be reworked.\n>\n\nCould you briefly describe how the new APIs would be like?\n\nBasically I think either applications need to have an argument to indicate\nwhich kind of stream the StreamConfiguration needs to be assigned to, or\nthe pipeline handler needs to remember which StreamRole a\nStreamConfiguration returned was asked as the default configuration for.\nIt doesn't have to be `StreamRole`. If there are other types/arguments in\nthe new reworked API, I'd be happy to adapt to the new one(s).\n\nTwo naive ideas:\n1. Keep the new `StreamRole role;` to be a const member variable in\nStreamConfiguration, which should be set in\n`PipelineHandler::generateConfiguration()`.\n\n2. If it makes sense, add a private class for `StreamConfiguration` and keep\nthe new `StreamRole role;` there, so that applications cannot get the info.\n(I don't think we need to hide the info from the applications though...)\n\nWDYT?\n\n\n> Cc-Laurent for opinions.\n>\n> >\n> > BR,\n> > Harvey\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 80BD3C3257\n\tfor ;\n\tThu, 19 Sep 2024 13:56:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 34F63634FC;\n\tThu, 19 Sep 2024 15:56:39 +0200 (CEST)","from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2BABD618E7\n\tfor ;\n\tThu, 19 Sep 2024 15:56:37 +0200 (CEST)","by mail-lf1-x130.google.com with SMTP id\n\t2adb3069b0e04-536552fcc07so57628e87.1\n\tfor ;\n\tThu, 19 Sep 2024 06:56:37 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"lZjZavgK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=google.com; s=20230601; t=1726754196; x=1727358996;\n\tdarn=lists.libcamera.org; \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=sdUg/uTSLnSGTMdqNOsgDs1ztNfSijX9ItLbCXke1Fc=;\n\tb=lZjZavgKI3mJdf7nVJ5l85NsiizIMMWPcW+boJ1eUxfxrWAj37TKNYSmVkUOUskYm2\n\trKn7SDpous32qePDGWD7ABmanQiNbX+pTESJn5E033wajSFAHe1V4sSuFeUdiCbxOfGh\n\tYXDXrv2yPbo1sfuXxs7p3j2J6oMlOyz4zGctGapWUVYV53fnK0mGemMhQcYpPOgON0KJ\n\tngv724WYNg7sFCAxgE252hPG4xYPdRxA7r7U3aVXHxIVdfHr8N23xMyQmvSwnhY9HVPP\n\tjTueik6aDekwOd2Ap6goMzAVhRMNc70R+eflBQtSqeppFT/aWhjeWkOTCQWzU5n0Qfk2\n\txi0Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1726754196; x=1727358996;\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=sdUg/uTSLnSGTMdqNOsgDs1ztNfSijX9ItLbCXke1Fc=;\n\tb=ByHmnkgttlwe4Z5JmIPFoP1CywUiq2W7mkPQX0AF80+u4Y3ECypue4+ZlqzFjrPyFa\n\tiVrPllXkEWS8ewg+CK4bueQKk7ibPAZywbRSER88P//NqpLx17E5UPF757Uap+caEjqH\n\twAgUOpHsJXOmkNiqXVXHDU3gAl7Tnn2S5S5diWMQOEnTbEsyx5o3E6FvuG+GHlV8IJKV\n\tusbOpWcZQ0MDHnawH27uLywCjsB3bwEj/kJr5Pv6EYfvtHXqHCP3oaQXILOHBYL+OTX5\n\t+rmxMVELdHsfXqqQvYaA2Tm5W9L+SHbt+zgzEWeMBRUWWlt5PhNCnaEUGHWi8vmMrQtG\n\tEDBw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUZBmTqGj7QS/QGoP4eFgvYHijNrtBfEIQtt6P6p4t45Oj7HxlKJ1NOmCOgIl8jS1gyAIn2MYBjWCQMMrz+9jA=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yy9CUEjJ6MES6J8q5LaVPGllPgD7zVFsNVQHdsJzvoA0zNNb1GN\n\tJAZg6ZD2TnCSoxRoa7IMJ0wSu9lxalIhXcwYRB4guxiiCR1bD7BCP11uREdGnB+BM+PB3QmI/pr\n\tVglyvQItSpjdsU9VuuZT9oSy6iChvdh7x7zbM","X-Google-Smtp-Source":"AGHT+IEblwU8f2ndsg77WddP0vS+qn/uMWDJcYgnC8RbtDg404VD2Aa8xIFqPEgxUCBsyBetWTXgaCcnXX/D9SLwhfk=","X-Received":"by 2002:a05:6512:3f26:b0:52c:dd94:73f3 with SMTP id\n\t2adb3069b0e04-536a7bfa7b6mr267020e87.3.1726754195793; Thu, 19 Sep 2024\n\t06:56:35 -0700 (PDT)","MIME-Version":"1.0","References":"<20240916045802.3799103-1-chenghaoyang@google.com>\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Thu, 19 Sep 2024 21:55:58 +0800","Message-ID":"","Subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","To":"Jacopo Mondi ","Cc":"Cheng-Hao Yang ,\n\tlibcamera-devel@lists.libcamera.org, \n\tLaurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000007563a30622794aab\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31309,"web_url":"https://patchwork.libcamera.org/comment/31309/","msgid":"<5imcxykorjceprfzhynsijjian7p55fn5vry6ogfajsanczian@xtg6ooszbxo3>","date":"2024-09-23T12:56:58","subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Thu, Sep 19, 2024 at 09:55:58PM GMT, Cheng-Hao Yang wrote:\n> Hi Jacopo and Laurent,\n>\n> On Thu, Sep 19, 2024 at 5:09 PM Jacopo Mondi \n> wrote:\n>\n> > Hi Harvey\n> >\n> > On Wed, Sep 18, 2024 at 04:33:27PM GMT, Cheng-Hao Yang wrote:\n> > > Sorry Jacopo, my bad to miss the first message:\n> > >\n> > > On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi <\n> > jacopo.mondi@ideasonboard.com>\n> > > wrote:\n> > >\n> > > > Hi Harvey\n> > > >\n> > > > On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:\n> > > > > Hi Jacopo,\n> > > > >\n> > > > > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi <\n> > > > jacopo.mondi@ideasonboard.com>\n> > > > > wrote:\n> > > > >\n> > > > > > Hi Harvey\n> > > > > >\n> > > > > > On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:\n> > > > > > > Hi folks,\n> > > > > > >\n> > > > > > > Currently applications set resolutions, pixelFormat, bufferCount,\n> > > > etc,\n> > > > > > > into StreamConfigurations, and Pipeline Handler decides which\n> > streams\n> > > > > > > they're assigned to. However, it doesn't allow application to\n> > assign\n> > > > > > > streams that cannot be distinguished by those arguments into\n> > > > > > > VideoRecording or StillCapture (say YUV/NV12 format), which is\n> > > > needed in\n> > > > > > > mtkisp7.\n> > > > > >\n> > > > > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > > > > you plan to use StreamRole as part of the stream configuration ?\n> > > > > >\n> > > >\n> > > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > > you plan to use StreamRole as part of the stream configuration ?\n> > > >\n> > > >\n> > > >\n> > > In mtkisp7 pipeline handler, both preview (or video) and still capture\n> > > streams support the same format (NV12) and bufferCount, and the\n> > > maximum resolutions are also the same. Therefore, when calling\n> > > `validate()` and `configure()`, mtkisp7 pipeline handler doesn't know\n> > > if applications/Android adapter wants the stream to go through the\n> > > still captur pipeline or not.\n> >\n> > I still have an hard time understanding why validate() and configure()\n> > needs to know the \"role\". Is this to assign \"pipes\" to Streams ?\n>\n>\n> If I understand correctly that \"pipes\" means the pipelines of ISP/IPA algos\n> for preview/video/still capture respectively, yes, it's to assign the\n> StreamConfiguration(s) to the desired pipe(s), which means to call\n> `StreamConfiguration::setStream()`. According to the comments [1], it's\n> supposed to be called in `PipelineHandler::configure()`, like\n> SimplePipelineHandler [2]. However, there are also some pipeline handlers\n> that set them in `CameraConfiguration::validate()` [3] [4].\n\nIndeed most of our documentation suggests that setStream() is meant to\nbe called during configure(). Considering that validate() is -always-\ncalled before configure() by Camera, so where exactly you call it, I\ndon't think makes a big difference.\n\nWe could also relax the documentation.\n\n>\n> [1]:\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n303\n> [2]:\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/simple/simple.cpp#n1270\n> [3]:\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n347\n> [4]:\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rkisp1/rkisp1.cpp#n520\n>\n> Does\n> > the mtkisp7 pipes have different capabilities when it comes to\n> > supported formats and resolutions ?\n> >\n>\n> No, as you can see in mtkisp7's implementation [5], all of them support the\n> same formats and resolutions.\n>\n> [5]:\n> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n>\n>\n>\n>\n> > >\n> > > Does that make sense :P?\n> >\n> > I guess so, but I wonder if the \"role\" is the main criteria that\n> > should be taken into account when assiging pipes to streams.\n> >\n> How would this work for applications that operates on platforms where\n> > the pipe selection depends on other criteria like the supported image\n> > formats and stream resolutions ? In example, for rkisp1\n> > the \"self\" pipelines can do RGB while the \"main\" can't and that's how\n> > we assign pipes during validate().\n>\n>\n> > To be honest this has not proven to be optimal and I'm not opposed to\n> > add Roles to the pipe selection criteria, but they shouldn't be made the\n> > only criteria on which pipes are assigned (unless we support this on\n> > all pipelines).\n> >\n> >\n> I agree that the `role` might not be the main criteria in general cases.\n> It's just that we find difficulties in assigning them properly with the\n> current\n> configurations in mtkisp7.\n>\n>\n>\n> > Also I would not based any future-proof design on Roles, they will\n> > likely be heavily reworked or removed going forward.\n> >\n> > As your main use case is Android, I think it would be doable for you\n> > to\n> > 1) Request a generateConfiguration() and keep track of the\n> > StreamConfiguration order.\n> >\n> > generateConfiguration(Viewfinder, Still, Raw)\n> >\n> > will give you StreamConfiguration for the above rols in order as\n> > you have requested them, if supported.\n> >\n> > 2) Code validate() so that you try to assing pipes based on the\n> > formats/sizes and if formats/sizes are the same define an ordering.\n> > In example the first YUV/WxH stream goes to Viewfinder if an\n> > identical YUV/WxH stream is requested for Still Capture.\n> >\n>\n> This actually assumes the application doesn't change the content of the\n> default CameraConfiguration/StreamConfiguration, which I doubt if it's a\n> good thing. Currently in Android adapter, it re-arranges [6]\n> StreamConfigurations based on the previously-fetched default\n> StreamConfigurations of the StreamRoles, which I think is a normal\n> practice of the current interfaces' logic. Please correct me if I'm wrong :)\n\nGood point, as Android HAL re-sorts the StreamConfiguration before\ncalling Camera::configure() you can't rely on the order in which you\nhave requested roles to Camera::generateConfiguration().\n\n>\n> [6]:\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n708\n>\n> Also, if an application calls\n> `generateConfiguration(camera, {StreamRole::VideoRecording})` and\n> `generateConfiguration(camera, {StreamRole::StillCapture})`\n> respectively, and ends up calling `configure()` with `VideoRecording`'s\n> CameraConfiguration, how would the pipeline handler know that the\n> application intends to get buffers from the stream `VideoRecording`?\n> In your suggestion, it seems that the application needs to call\n> `generateConfiguration(camera, {StreamRole::VideoRecording})` again,\n> which I don't think is enforced in the current libcamera API, and might\n> not be a good practice.\n>\n>\n> > 3) As validate() assigns Steams * to StreamConfiguration (with\n> > ::setStream) it should be easy to keep track to which pipe a\n> > StreamConfiguration has been assigned to at configure() time.\n> >\n> > Could you list what are the platform's pipes capabilities ?\n> >\n> >\n> For example, on `ciri` with mtkisp7 pipeline handler [5], `pixelFormat` is\n> NV12,\n> supported resolution range is `320x240` to `2592x1944`, and `bufferCount` is\n> 8. It applies to `StillCapture`, `VideoRecording`, and `ViewFinder`.\n>\n> [5]:\n> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n>\n\nLooking at the pipeline handler implementation from your tree it seems\nto me that you're using roles in validate() to call setStream(), as we\nalready clarified:\n\n\t\tswitch (cfg.role) {\n\t\tcase StreamRole::Viewfinder:\n\t\tcase StreamRole::VideoRecording:\n\t\t\tif (videoCnt >= 2) {\n\t\t\t\tLOG(MtkISP7, Error)\n\t\t\t\t\t<< \"Support only 2 Preview/Video streams\";\n\t\t\t\treturn Invalid;\n\t\t\t}\n\t\t\tcfg.setStream(const_cast(vidStreams[videoCnt++]));\n\t\t\tbreak;\n\t\tcase StreamRole::StillCapture:\n\t\t\tif (stillCnt >= 2) {\n\t\t\t\tLOG(MtkISP7, Error)\n\t\t\t\t\t<< \"Support only 2 StillCapture streams\";\n\t\t\t\treturn Invalid;\n\t\t\t}\n\t\t\tcfg.setStream(const_cast(stillStreams[stillCnt++]));\n\t\t\tbreak;\n\t\tdefault:\n\t\t\tLOG(MtkISP7, Error) << \"Invalid StreamRole \" << cfg.role;\n\t\t\treturn Invalid;\n\t\t}\n\nThis shows that you have 4 streams, 2 for Preview/Video and 2 for\nStillCapture.\n\nThen you use the Stream in configure() for populate two sizes\n\n\tSize video1 = Size{ 0, 0 };\n\tSize video2 = Size{ 0, 0 };\n\tSize still1 = Size{ 0, 0 };\n\tSize still2 = Size{ 0, 0 };\n\n\t/* Only cover the video resolution */\n\tfor (auto &cfg : *c) {\n\t\tif (cfg.stream() == &video1Stream_)\n\t\t\tvideo1 = cfg.size;\n\t\telse if (cfg.stream() == &video2Stream_)\n\t\t\tvideo2 = cfg.size;\n\t\telse if (cfg.stream() == &still1Stream_)\n\t\t\tstill1 = cfg.size;\n\t\telse if (cfg.stream() == &still2Stream_)\n\t\t\tstill2 = cfg.size;\n\t\telse\n\t\t\treturn -EINVAL;\n\t}\n\nFrom there on, all the API you have implemented relies on the presence\nand order of the 'video1', 'video2, 'still1' and 'still2' parameters.\n\nIn example\n\n\tmcnrManager.configure(camsysYuvSize, video1, video2);\n\tlpnrManager.configure(sensorFullSize_, still1, still2);\n\tlpnrTunManager.configure(sensorFullSize_, still1, still2);\n\tmcnrTunManager.configure(camsysYuvSize, video1, video2);\n\nNow, according to what I've read and what you said, there are no\nconstraints on the HW that help identify Stream. To make an example,\nyou can't assume \"Oh this StreamConfiguration is RGB so it needs to go\nto StreamX\".\n\nSo I guess what you want is to allow an application to say \"One\nviewfinder is WxH, one Still capture is WxH, one viewfinder is WxH\"\n(also, all Streams are NV12 if I'm not mistaken).\n\n> As said, I'm not opposed to use Roles for assigning pipes, but I don't\n> > think we should based any new development on Roles as there's an high\n> > chance they can be reworked.\n> >\n>\n> Could you briefly describe how the new APIs would be like?\n>\n> Basically I think either applications need to have an argument to indicate\n> which kind of stream the StreamConfiguration needs to be assigned to, or\n> the pipeline handler needs to remember which StreamRole a\n> StreamConfiguration returned was asked as the default configuration for.\n\nIt's not really about that, the idea is that the stream's\ncharacteristics should be used to assign a stream to a pipe, like the\nformat and the sizes. In your case that's not really possible as all\nstreams are NV12 and all resolutions can go anywhere.\n\nBut, as you use roles, that logic should live somewhere, doesn't it ?\n\nLooking at your implementation of the Android HAL I see (sorry, cannot\npoint out a commit id as there's quite some reverts in the history)\n\n\t\tif (isJpegStream(stream)) {\n\t\t\tcontinue;\n\t\t} else if (isYuvSnapshotStream(stream)) {\n\t\t\tstreamConfig.streams = { { stream, CameraStream::Type::Direct } };\n\t\t\tstreamConfig.config.role = StreamRole::StillCapture;\n\t\t} else if (isPreviewStream(stream)) {\n\t\t\tstreamConfig.streams = { { stream, CameraStream::Type::Direct } };\n\t\t\tstreamConfig.config.role = StreamRole::Viewfinder;\n\t\t} else if (isVideoStream(stream)) {\n\t\t\tstreamConfig.streams = { { stream, CameraStream::Type::Direct } };\n\t\t\tstreamConfig.config.role = StreamRole::VideoRecording;\n\t\t} else {\n\t\t\tstreamConfig.streams = { { stream, CameraStream::Type::Direct } };\n\t\t\tstreamConfig.config.role = StreamRole::Viewfinder;\n\t\t}\n\nSo you populate roles based on some criteria here, and if I look at\nthe criteria\n\nbool isPreviewStream(camera3_stream_t *stream)\n{\n\treturn (GRALLOC_USAGE_HW_COMPOSER & stream->usage);\n}\n\nbool isVideoStream(camera3_stream_t *stream)\n{\n\treturn (GRALLOC_USAGE_HW_VIDEO_ENCODER & stream->usage);\n}\n\nbool isYuvSnapshotStream(camera3_stream_t *stream)\n{\n\treturn (!isVideoStream(stream) && !isPreviewStream(stream) &&\n\t\t(HAL_PIXEL_FORMAT_YCbCr_420_888 == stream->format));\n}\n\nthey indeed come from Android's requirements that cannot be expressed\nin libcamera.\n\nTo be clear, when asking \"what you need this for\" this is the level of\ndetail that is needed to explain your design choices. Otherwise it's\nup to us to decode what you have done in the mtk support branch.\n\n\n> It doesn't have to be `StreamRole`. If there are other types/arguments in\n> the new reworked API, I'd be happy to adapt to the new one(s).\n>\n> Two naive ideas:\n> 1. Keep the new `StreamRole role;` to be a const member variable in\n> StreamConfiguration, which should be set in\n> `PipelineHandler::generateConfiguration()`.\n>\n> 2. If it makes sense, add a private class for `StreamConfiguration` and keep\n> the new `StreamRole role;` there, so that applications cannot get the info.\n> (I don't think we need to hide the info from the applications though...)\n>\n> WDYT?\n\nGiven the above described use case is my opinion valid, I wouldn't be\nopposed to have roles as a public member of the StreamConfiguration to\nallow application to hint how a stream should be used if no other\ncriteria is applicable.\n\n>\n>\n> > Cc-Laurent for opinions.\n\nAs this is an application-facing API change, I would however like to\nsee more opinions.\n\nThanks\n j\n\n> >\n> > >\n> > > BR,\n> > > Harvey\n> >\n>\n>\n> --\n> BR,\n> Harvey Yang","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 93173C3257\n\tfor ;\n\tMon, 23 Sep 2024 12:57:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8721C618DE;\n\tMon, 23 Sep 2024 14:57:04 +0200 (CEST)","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 65AC66037E\n\tfor ;\n\tMon, 23 Sep 2024 14:57:02 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A08C6161;\n\tMon, 23 Sep 2024 14:55:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"J8sNW47K\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727096135;\n\tbh=X2ttM9iV6vtvC4rT9a43zjW7uSRPfJKKinkVnSCi6ho=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=J8sNW47KRKEfxiwcbppVsw8A5FfJQizs24hwLmJW0yuiMR0jDA4wn9O9ZSUGjeHBp\n\tJZiYloEYkOspRCnA22nXJ308GsAdfdKv2AmqExW1MMEm6lcmz3bdzMxyzGJ1EE6hYL\n\tsNc6c9gCJPsI90fmOw56mHFbKea3Gh2GD6bGMtC8=","Date":"Mon, 23 Sep 2024 14:56:58 +0200","From":"Jacopo Mondi ","To":"Cheng-Hao Yang , \n\tLaurent Pinchart ,\n\tkieran bingham ","Cc":"Jacopo Mondi , \n\tCheng-Hao Yang ,\n\tlibcamera-devel@lists.libcamera.org, \n\tLaurent Pinchart ","Subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","Message-ID":"<5imcxykorjceprfzhynsijjian7p55fn5vry6ogfajsanczian@xtg6ooszbxo3>","References":"<20240916045802.3799103-1-chenghaoyang@google.com>\n\t\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31311,"web_url":"https://patchwork.libcamera.org/comment/31311/","msgid":"","date":"2024-09-23T14:14:48","subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","submitter":{"id":148,"url":"https://patchwork.libcamera.org/api/people/148/","name":"Cheng-Hao Yang","email":"chenghaoyang@google.com"},"content":"Thanks Jacopo for looking into the tree of the mtkisp7 branch!\n\n\nOn Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi \nwrote:\n\n> Hi Harvey\n>\n> On Thu, Sep 19, 2024 at 09:55:58PM GMT, Cheng-Hao Yang wrote:\n> > Hi Jacopo and Laurent,\n> >\n> > On Thu, Sep 19, 2024 at 5:09 PM Jacopo Mondi <\n> jacopo.mondi@ideasonboard.com>\n> > wrote:\n> >\n> > > Hi Harvey\n> > >\n> > > On Wed, Sep 18, 2024 at 04:33:27PM GMT, Cheng-Hao Yang wrote:\n> > > > Sorry Jacopo, my bad to miss the first message:\n> > > >\n> > > > On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi <\n> > > jacopo.mondi@ideasonboard.com>\n> > > > wrote:\n> > > >\n> > > > > Hi Harvey\n> > > > >\n> > > > > On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:\n> > > > > > Hi Jacopo,\n> > > > > >\n> > > > > > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi <\n> > > > > jacopo.mondi@ideasonboard.com>\n> > > > > > wrote:\n> > > > > >\n> > > > > > > Hi Harvey\n> > > > > > >\n> > > > > > > On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:\n> > > > > > > > Hi folks,\n> > > > > > > >\n> > > > > > > > Currently applications set resolutions, pixelFormat,\n> bufferCount,\n> > > > > etc,\n> > > > > > > > into StreamConfigurations, and Pipeline Handler decides which\n> > > streams\n> > > > > > > > they're assigned to. However, it doesn't allow application to\n> > > assign\n> > > > > > > > streams that cannot be distinguished by those arguments into\n> > > > > > > > VideoRecording or StillCapture (say YUV/NV12 format), which\n> is\n> > > > > needed in\n> > > > > > > > mtkisp7.\n> > > > > > >\n> > > > > > > Could you explain in a bit more detail why this \"is needed\"\n> and how\n> > > > > > > you plan to use StreamRole as part of the stream configuration\n> ?\n> > > > > > >\n> > > > >\n> > > > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > > > you plan to use StreamRole as part of the stream configuration ?\n> > > > >\n> > > > >\n> > > > >\n> > > > In mtkisp7 pipeline handler, both preview (or video) and still\n> capture\n> > > > streams support the same format (NV12) and bufferCount, and the\n> > > > maximum resolutions are also the same. Therefore, when calling\n> > > > `validate()` and `configure()`, mtkisp7 pipeline handler doesn't know\n> > > > if applications/Android adapter wants the stream to go through the\n> > > > still captur pipeline or not.\n> > >\n> > > I still have an hard time understanding why validate() and configure()\n> > > needs to know the \"role\". Is this to assign \"pipes\" to Streams ?\n> >\n> >\n> > If I understand correctly that \"pipes\" means the pipelines of ISP/IPA\n> algos\n> > for preview/video/still capture respectively, yes, it's to assign the\n> > StreamConfiguration(s) to the desired pipe(s), which means to call\n> > `StreamConfiguration::setStream()`. According to the comments [1], it's\n> > supposed to be called in `PipelineHandler::configure()`, like\n> > SimplePipelineHandler [2]. However, there are also some pipeline handlers\n> > that set them in `CameraConfiguration::validate()` [3] [4].\n>\n> Indeed most of our documentation suggests that setStream() is meant to\n> be called during configure(). Considering that validate() is -always-\n> called before configure() by Camera, so where exactly you call it, I\n> don't think makes a big difference.\n>\n> We could also relax the documentation.\n>\n\nYes, that's a great idea to avoid confusion.\n\n\n>\n> >\n> > [1]:\n> >\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n303\n> > [2]:\n> >\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/simple/simple.cpp#n1270\n> > [3]:\n> >\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n347\n> > [4]:\n> >\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rkisp1/rkisp1.cpp#n520\n> >\n> > Does\n> > > the mtkisp7 pipes have different capabilities when it comes to\n> > > supported formats and resolutions ?\n> > >\n> >\n> > No, as you can see in mtkisp7's implementation [5], all of them support\n> the\n> > same formats and resolutions.\n> >\n> > [5]:\n> >\n> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n> >\n> >\n> >\n> >\n> > > >\n> > > > Does that make sense :P?\n> > >\n> > > I guess so, but I wonder if the \"role\" is the main criteria that\n> > > should be taken into account when assiging pipes to streams.\n> > >\n> > How would this work for applications that operates on platforms where\n> > > the pipe selection depends on other criteria like the supported image\n> > > formats and stream resolutions ? In example, for rkisp1\n> > > the \"self\" pipelines can do RGB while the \"main\" can't and that's how\n> > > we assign pipes during validate().\n> >\n> >\n> > > To be honest this has not proven to be optimal and I'm not opposed to\n> > > add Roles to the pipe selection criteria, but they shouldn't be made\n> the\n> > > only criteria on which pipes are assigned (unless we support this on\n> > > all pipelines).\n> > >\n> > >\n> > I agree that the `role` might not be the main criteria in general cases.\n> > It's just that we find difficulties in assigning them properly with the\n> > current\n> > configurations in mtkisp7.\n> >\n> >\n> >\n> > > Also I would not based any future-proof design on Roles, they will\n> > > likely be heavily reworked or removed going forward.\n> > >\n> > > As your main use case is Android, I think it would be doable for you\n> > > to\n> > > 1) Request a generateConfiguration() and keep track of the\n> > > StreamConfiguration order.\n> > >\n> > > generateConfiguration(Viewfinder, Still, Raw)\n> > >\n> > > will give you StreamConfiguration for the above rols in order as\n> > > you have requested them, if supported.\n> > >\n> > > 2) Code validate() so that you try to assing pipes based on the\n> > > formats/sizes and if formats/sizes are the same define an ordering.\n> > > In example the first YUV/WxH stream goes to Viewfinder if an\n> > > identical YUV/WxH stream is requested for Still Capture.\n> > >\n> >\n> > This actually assumes the application doesn't change the content of the\n> > default CameraConfiguration/StreamConfiguration, which I doubt if it's a\n> > good thing. Currently in Android adapter, it re-arranges [6]\n> > StreamConfigurations based on the previously-fetched default\n> > StreamConfigurations of the StreamRoles, which I think is a normal\n> > practice of the current interfaces' logic. Please correct me if I'm\n> wrong :)\n>\n> Good point, as Android HAL re-sorts the StreamConfiguration before\n> calling Camera::configure() you can't rely on the order in which you\n> have requested roles to Camera::generateConfiguration().\n>\n> >\n> > [6]:\n> >\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n708\n> >\n> > Also, if an application calls\n> > `generateConfiguration(camera, {StreamRole::VideoRecording})` and\n> > `generateConfiguration(camera, {StreamRole::StillCapture})`\n> > respectively, and ends up calling `configure()` with `VideoRecording`'s\n> > CameraConfiguration, how would the pipeline handler know that the\n> > application intends to get buffers from the stream `VideoRecording`?\n> > In your suggestion, it seems that the application needs to call\n> > `generateConfiguration(camera, {StreamRole::VideoRecording})` again,\n> > which I don't think is enforced in the current libcamera API, and might\n> > not be a good practice.\n> >\n> >\n> > > 3) As validate() assigns Steams * to StreamConfiguration (with\n> > > ::setStream) it should be easy to keep track to which pipe a\n> > > StreamConfiguration has been assigned to at configure() time.\n> > >\n> > > Could you list what are the platform's pipes capabilities ?\n> > >\n> > >\n> > For example, on `ciri` with mtkisp7 pipeline handler [5], `pixelFormat`\n> is\n> > NV12,\n> > supported resolution range is `320x240` to `2592x1944`, and\n> `bufferCount` is\n> > 8. It applies to `StillCapture`, `VideoRecording`, and `ViewFinder`.\n> >\n> > [5]:\n> >\n> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n> >\n>\n> Looking at the pipeline handler implementation from your tree it seems\n> to me that you're using roles in validate() to call setStream(), as we\n> already clarified:\n>\n> switch (cfg.role) {\n> case StreamRole::Viewfinder:\n> case StreamRole::VideoRecording:\n> if (videoCnt >= 2) {\n> LOG(MtkISP7, Error)\n> << \"Support only 2 Preview/Video\n> streams\";\n> return Invalid;\n> }\n> cfg.setStream(const_cast *>(vidStreams[videoCnt++]));\n> break;\n> case StreamRole::StillCapture:\n> if (stillCnt >= 2) {\n> LOG(MtkISP7, Error)\n> << \"Support only 2 StillCapture\n> streams\";\n> return Invalid;\n> }\n> cfg.setStream(const_cast *>(stillStreams[stillCnt++]));\n> break;\n> default:\n> LOG(MtkISP7, Error) << \"Invalid StreamRole \" <<\n> cfg.role;\n> return Invalid;\n> }\n>\n> This shows that you have 4 streams, 2 for Preview/Video and 2 for\n> StillCapture.\n>\n> Then you use the Stream in configure() for populate two sizes\n>\n> Size video1 = Size{ 0, 0 };\n> Size video2 = Size{ 0, 0 };\n> Size still1 = Size{ 0, 0 };\n> Size still2 = Size{ 0, 0 };\n>\n> /* Only cover the video resolution */\n> for (auto &cfg : *c) {\n> if (cfg.stream() == &video1Stream_)\n> video1 = cfg.size;\n> else if (cfg.stream() == &video2Stream_)\n> video2 = cfg.size;\n> else if (cfg.stream() == &still1Stream_)\n> still1 = cfg.size;\n> else if (cfg.stream() == &still2Stream_)\n> still2 = cfg.size;\n> else\n> return -EINVAL;\n> }\n>\n> From there on, all the API you have implemented relies on the presence\n> and order of the 'video1', 'video2, 'still1' and 'still2' parameters.\n>\n> In example\n>\n> mcnrManager.configure(camsysYuvSize, video1, video2);\n> lpnrManager.configure(sensorFullSize_, still1, still2);\n> lpnrTunManager.configure(sensorFullSize_, still1, still2);\n> mcnrTunManager.configure(camsysYuvSize, video1, video2);\n>\n> Now, according to what I've read and what you said, there are no\n> constraints on the HW that help identify Stream. To make an example,\n> you can't assume \"Oh this StreamConfiguration is RGB so it needs to go\n> to StreamX\".\n>\n\nYes, that's the main concern :)\n\n\n>\n> So I guess what you want is to allow an application to say \"One\n> viewfinder is WxH, one Still capture is WxH, one viewfinder is WxH\"\n> (also, all Streams are NV12 if I'm not mistaken).\n>\n> > As said, I'm not opposed to use Roles for assigning pipes, but I don't\n> > > think we should based any new development on Roles as there's an high\n> > > chance they can be reworked.\n> > >\n> >\n> > Could you briefly describe how the new APIs would be like?\n> >\n> > Basically I think either applications need to have an argument to\n> indicate\n> > which kind of stream the StreamConfiguration needs to be assigned to, or\n> > the pipeline handler needs to remember which StreamRole a\n> > StreamConfiguration returned was asked as the default configuration for.\n>\n> It's not really about that, the idea is that the stream's\n> characteristics should be used to assign a stream to a pipe, like the\n> format and the sizes. In your case that's not really possible as all\n> streams are NV12 and all resolutions can go anywhere.\n>\n> But, as you use roles, that logic should live somewhere, doesn't it ?\n>\n> Looking at your implementation of the Android HAL I see (sorry, cannot\n> point out a commit id as there's quite some reverts in the history)\n>\n> if (isJpegStream(stream)) {\n> continue;\n> } else if (isYuvSnapshotStream(stream)) {\n> streamConfig.streams = { { stream,\n> CameraStream::Type::Direct } };\n> streamConfig.config.role =\n> StreamRole::StillCapture;\n> } else if (isPreviewStream(stream)) {\n> streamConfig.streams = { { stream,\n> CameraStream::Type::Direct } };\n> streamConfig.config.role = StreamRole::Viewfinder;\n> } else if (isVideoStream(stream)) {\n> streamConfig.streams = { { stream,\n> CameraStream::Type::Direct } };\n> streamConfig.config.role =\n> StreamRole::VideoRecording;\n> } else {\n> streamConfig.streams = { { stream,\n> CameraStream::Type::Direct } };\n> streamConfig.config.role = StreamRole::Viewfinder;\n> }\n>\n> So you populate roles based on some criteria here, and if I look at\n> the criteria\n>\n> bool isPreviewStream(camera3_stream_t *stream)\n> {\n> return (GRALLOC_USAGE_HW_COMPOSER & stream->usage);\n> }\n>\n> bool isVideoStream(camera3_stream_t *stream)\n> {\n> return (GRALLOC_USAGE_HW_VIDEO_ENCODER & stream->usage);\n> }\n>\n> bool isYuvSnapshotStream(camera3_stream_t *stream)\n> {\n> return (!isVideoStream(stream) && !isPreviewStream(stream) &&\n> (HAL_PIXEL_FORMAT_YCbCr_420_888 == stream->format));\n> }\n>\n> they indeed come from Android's requirements that cannot be expressed\n> in libcamera.\n>\n> To be clear, when asking \"what you need this for\" this is the level of\n> detail that is needed to explain your design choices. Otherwise it's\n> up to us to decode what you have done in the mtk support branch.\n>\n\nI think that's also related to another confusion that I have regarding the\ncurrent libcamera API: We use `StreamRole` as the input to get the\ndefault configuration, while it doesn't stop a pipeline handler to assign\nit to a different stream, with a different StreamRole, later in `validate()`\nor `configure()`. The pipeline handler might not even return\n`Status::Adjusted`, if the requested arguments are not changed.\n\nMaybe libcamera core libraries assume each ISP pipe has different\ncharacteristics, like PixelFormat?\n\n\n>\n>\n> > It doesn't have to be `StreamRole`. If there are other types/arguments in\n> > the new reworked API, I'd be happy to adapt to the new one(s).\n> >\n> > Two naive ideas:\n> > 1. Keep the new `StreamRole role;` to be a const member variable in\n> > StreamConfiguration, which should be set in\n> > `PipelineHandler::generateConfiguration()`.\n> >\n> > 2. If it makes sense, add a private class for `StreamConfiguration` and\n> keep\n> > the new `StreamRole role;` there, so that applications cannot get the\n> info.\n> > (I don't think we need to hide the info from the applications though...)\n> >\n> > WDYT?\n>\n> Given the above described use case is my opinion valid, I wouldn't be\n> opposed to have roles as a public member of the StreamConfiguration to\n> allow application to hint how a stream should be used if no other\n> criteria is applicable.\n>\n\nYeah I also think that the current patch is the simplest.\n\n\n>\n> >\n> >\n> > > Cc-Laurent for opinions.\n>\n> As this is an application-facing API change, I would however like to\n> see more opinions.\n>\n\nSure, let's wait for more opinions from others :)\n\n\n>\n> Thanks\n> j\n>\n> > >\n> > > >\n> > > > BR,\n> > > > Harvey\n> > >\n> >\n> >\n> > --\n> > BR,\n> > Harvey Yang\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 52E15C3257\n\tfor ;\n\tMon, 23 Sep 2024 14:15:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E56A6350B;\n\tMon, 23 Sep 2024 16:15:30 +0200 (CEST)","from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com\n\t[IPv6:2a00:1450:4864:20::12d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9D1776037E\n\tfor ;\n\tMon, 23 Sep 2024 16:15:27 +0200 (CEST)","by mail-lf1-x12d.google.com with SMTP id\n\t2adb3069b0e04-5356a2460ceso27307e87.0\n\tfor ;\n\tMon, 23 Sep 2024 07:15:27 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"AWjH1Mcf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=google.com; s=20230601; t=1727100927; x=1727705727;\n\tdarn=lists.libcamera.org; \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=oExVZR++F55pmwXjzzkJUqfviU04agf7+tba6klhlLc=;\n\tb=AWjH1McfO5EJvCO47jGMdrmv34ZyfOQBgeW2CFXQ2mOkETNtkGSSLEnfM3lGlTozF5\n\tueyP5ctvjwEh7cz8/4p64QkpBVz8xHpTzZbHGo7A7U7zaVtw51K1MbaXiQSbK3VqFyKG\n\tYgeymuZAKOCPLw+mtaQalKXL3JFKFCGSf0YPiAKQwRMIua1VHnI0nZphQ8aJJPQPHXET\n\tXEg14zafE1127hf45KMURCU0Jgw29q6x5vlsNNKMdKTULhjvRCgMtzLln9j+nN/1ow/z\n\tRUn/372y8mOvam4Ye1dzTPzXA/VV6/qHqm2kTHYwc6Qlnw/DJJWs/uRjHH8Y7o00WKda\n\tH2aQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1727100927; x=1727705727;\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=oExVZR++F55pmwXjzzkJUqfviU04agf7+tba6klhlLc=;\n\tb=ESBdg1KABAqpns4sL+F1LXfc00yTQJH14aKuKazIkMsEuXsSxx8y6TxN/TYaAiTMDh\n\teYCvXUaMyZPKuVvYS0ltRslecL58JffpEUwL5ItUqM471cy9NuGasb8TpE19RJCHR/zZ\n\t/6zIeICiOOB61ZQJjZFymtTDlS9ygoowox9nNudpiIiysmUpWsqyFcHn3Y5ua8xZCzPX\n\tGnWHtpCi9FtCOjo25a1sOv2zy2kAv/gYsbjaAOQCqCaP8vKNA0trhwkU+Y0dnfXNskeE\n\tOqVaCnbnbO9wlxDiCX1/i6LUy0wWqB7EIIcXQ59T6hStqHdEp8PwnMcLYovBNwWWpPWf\n\tABsw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUqwxfhy4ZT2WT+fc6VsOMJ6oQsOCPKLcyhbfjmOXVGm25W0yjbqFBJFf/6VzakLsJ+RbAvayDLoH0G+HoDQ9k=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YxZ4K4rLM/PKyezb5gxjQ+l4j/eoW3DSNsWgGDLegAULArGKT/z\n\tRcenTXtMyWBLogt5eWUtI7MC7szkO3a95QqDWVFE7XABbT+flOEHalhbnsNgal4dpx/ufcx+/kN\n\tbeJHWq9x8QHAHiEpaXvQOc9ZnSlCNvOEftQjx","X-Google-Smtp-Source":"AGHT+IEdCQ8JjRluWOK53r8oriokcUxjnnd2TdSNiEyuZ+AXL/6MMHEcBSCmrIXwdVVVRefU6hgAL9jalFt4AKjfoEs=","X-Received":"by 2002:a05:6512:3085:b0:530:baaa:ee10 with SMTP id\n\t2adb3069b0e04-53799615935mr381344e87.3.1727100926206; Mon, 23 Sep 2024\n\t07:15:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20240916045802.3799103-1-chenghaoyang@google.com>\n\t\n\t\n\t\n\t\n\t\n\t\n\t<5imcxykorjceprfzhynsijjian7p55fn5vry6ogfajsanczian@xtg6ooszbxo3>","In-Reply-To":"<5imcxykorjceprfzhynsijjian7p55fn5vry6ogfajsanczian@xtg6ooszbxo3>","From":"Cheng-Hao Yang ","Date":"Mon, 23 Sep 2024 22:14:48 +0800","Message-ID":"","Subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","To":"Jacopo Mondi ","Cc":"Laurent Pinchart , \n\tkieran bingham , \n\tCheng-Hao Yang ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"multipart/alternative; boundary=\"0000000000003385bb0622ca054f\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31315,"web_url":"https://patchwork.libcamera.org/comment/31315/","msgid":"","date":"2024-09-23T15:10:15","subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Harvey\n\nOn Mon, Sep 23, 2024 at 10:14:48PM GMT, Cheng-Hao Yang wrote:\n> Thanks Jacopo for looking into the tree of the mtkisp7 branch!\n>\n>\n> On Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi \n> wrote:\n>\n> > Hi Harvey\n> >\n> > On Thu, Sep 19, 2024 at 09:55:58PM GMT, Cheng-Hao Yang wrote:\n> > > Hi Jacopo and Laurent,\n> > >\n> > > On Thu, Sep 19, 2024 at 5:09 PM Jacopo Mondi <\n> > jacopo.mondi@ideasonboard.com>\n> > > wrote:\n> > >\n> > > > Hi Harvey\n> > > >\n> > > > On Wed, Sep 18, 2024 at 04:33:27PM GMT, Cheng-Hao Yang wrote:\n> > > > > Sorry Jacopo, my bad to miss the first message:\n> > > > >\n> > > > > On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi <\n> > > > jacopo.mondi@ideasonboard.com>\n> > > > > wrote:\n> > > > >\n> > > > > > Hi Harvey\n> > > > > >\n> > > > > > On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:\n> > > > > > > Hi Jacopo,\n> > > > > > >\n> > > > > > > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi <\n> > > > > > jacopo.mondi@ideasonboard.com>\n> > > > > > > wrote:\n> > > > > > >\n> > > > > > > > Hi Harvey\n> > > > > > > >\n> > > > > > > > On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:\n> > > > > > > > > Hi folks,\n> > > > > > > > >\n> > > > > > > > > Currently applications set resolutions, pixelFormat,\n> > bufferCount,\n> > > > > > etc,\n> > > > > > > > > into StreamConfigurations, and Pipeline Handler decides which\n> > > > streams\n> > > > > > > > > they're assigned to. However, it doesn't allow application to\n> > > > assign\n> > > > > > > > > streams that cannot be distinguished by those arguments into\n> > > > > > > > > VideoRecording or StillCapture (say YUV/NV12 format), which\n> > is\n> > > > > > needed in\n> > > > > > > > > mtkisp7.\n> > > > > > > >\n> > > > > > > > Could you explain in a bit more detail why this \"is needed\"\n> > and how\n> > > > > > > > you plan to use StreamRole as part of the stream configuration\n> > ?\n> > > > > > > >\n> > > > > >\n> > > > > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > > > > you plan to use StreamRole as part of the stream configuration ?\n> > > > > >\n> > > > > >\n> > > > > >\n> > > > > In mtkisp7 pipeline handler, both preview (or video) and still\n> > capture\n> > > > > streams support the same format (NV12) and bufferCount, and the\n> > > > > maximum resolutions are also the same. Therefore, when calling\n> > > > > `validate()` and `configure()`, mtkisp7 pipeline handler doesn't know\n> > > > > if applications/Android adapter wants the stream to go through the\n> > > > > still captur pipeline or not.\n> > > >\n> > > > I still have an hard time understanding why validate() and configure()\n> > > > needs to know the \"role\". Is this to assign \"pipes\" to Streams ?\n> > >\n> > >\n> > > If I understand correctly that \"pipes\" means the pipelines of ISP/IPA\n> > algos\n> > > for preview/video/still capture respectively, yes, it's to assign the\n> > > StreamConfiguration(s) to the desired pipe(s), which means to call\n> > > `StreamConfiguration::setStream()`. According to the comments [1], it's\n> > > supposed to be called in `PipelineHandler::configure()`, like\n> > > SimplePipelineHandler [2]. However, there are also some pipeline handlers\n> > > that set them in `CameraConfiguration::validate()` [3] [4].\n> >\n> > Indeed most of our documentation suggests that setStream() is meant to\n> > be called during configure(). Considering that validate() is -always-\n> > called before configure() by Camera, so where exactly you call it, I\n> > don't think makes a big difference.\n> >\n> > We could also relax the documentation.\n> >\n>\n> Yes, that's a great idea to avoid confusion.\n>\n>\n> >\n> > >\n> > > [1]:\n> > >\n> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n303\n> > > [2]:\n> > >\n> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/simple/simple.cpp#n1270\n> > > [3]:\n> > >\n> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n347\n> > > [4]:\n> > >\n> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rkisp1/rkisp1.cpp#n520\n> > >\n> > > Does\n> > > > the mtkisp7 pipes have different capabilities when it comes to\n> > > > supported formats and resolutions ?\n> > > >\n> > >\n> > > No, as you can see in mtkisp7's implementation [5], all of them support\n> > the\n> > > same formats and resolutions.\n> > >\n> > > [5]:\n> > >\n> > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n> > >\n> > >\n> > >\n> > >\n> > > > >\n> > > > > Does that make sense :P?\n> > > >\n> > > > I guess so, but I wonder if the \"role\" is the main criteria that\n> > > > should be taken into account when assiging pipes to streams.\n> > > >\n> > > How would this work for applications that operates on platforms where\n> > > > the pipe selection depends on other criteria like the supported image\n> > > > formats and stream resolutions ? In example, for rkisp1\n> > > > the \"self\" pipelines can do RGB while the \"main\" can't and that's how\n> > > > we assign pipes during validate().\n> > >\n> > >\n> > > > To be honest this has not proven to be optimal and I'm not opposed to\n> > > > add Roles to the pipe selection criteria, but they shouldn't be made\n> > the\n> > > > only criteria on which pipes are assigned (unless we support this on\n> > > > all pipelines).\n> > > >\n> > > >\n> > > I agree that the `role` might not be the main criteria in general cases.\n> > > It's just that we find difficulties in assigning them properly with the\n> > > current\n> > > configurations in mtkisp7.\n> > >\n> > >\n> > >\n> > > > Also I would not based any future-proof design on Roles, they will\n> > > > likely be heavily reworked or removed going forward.\n> > > >\n> > > > As your main use case is Android, I think it would be doable for you\n> > > > to\n> > > > 1) Request a generateConfiguration() and keep track of the\n> > > > StreamConfiguration order.\n> > > >\n> > > > generateConfiguration(Viewfinder, Still, Raw)\n> > > >\n> > > > will give you StreamConfiguration for the above rols in order as\n> > > > you have requested them, if supported.\n> > > >\n> > > > 2) Code validate() so that you try to assing pipes based on the\n> > > > formats/sizes and if formats/sizes are the same define an ordering.\n> > > > In example the first YUV/WxH stream goes to Viewfinder if an\n> > > > identical YUV/WxH stream is requested for Still Capture.\n> > > >\n> > >\n> > > This actually assumes the application doesn't change the content of the\n> > > default CameraConfiguration/StreamConfiguration, which I doubt if it's a\n> > > good thing. Currently in Android adapter, it re-arranges [6]\n> > > StreamConfigurations based on the previously-fetched default\n> > > StreamConfigurations of the StreamRoles, which I think is a normal\n> > > practice of the current interfaces' logic. Please correct me if I'm\n> > wrong :)\n> >\n> > Good point, as Android HAL re-sorts the StreamConfiguration before\n> > calling Camera::configure() you can't rely on the order in which you\n> > have requested roles to Camera::generateConfiguration().\n> >\n> > >\n> > > [6]:\n> > >\n> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n708\n> > >\n> > > Also, if an application calls\n> > > `generateConfiguration(camera, {StreamRole::VideoRecording})` and\n> > > `generateConfiguration(camera, {StreamRole::StillCapture})`\n> > > respectively, and ends up calling `configure()` with `VideoRecording`'s\n> > > CameraConfiguration, how would the pipeline handler know that the\n> > > application intends to get buffers from the stream `VideoRecording`?\n> > > In your suggestion, it seems that the application needs to call\n> > > `generateConfiguration(camera, {StreamRole::VideoRecording})` again,\n> > > which I don't think is enforced in the current libcamera API, and might\n> > > not be a good practice.\n> > >\n> > >\n> > > > 3) As validate() assigns Steams * to StreamConfiguration (with\n> > > > ::setStream) it should be easy to keep track to which pipe a\n> > > > StreamConfiguration has been assigned to at configure() time.\n> > > >\n> > > > Could you list what are the platform's pipes capabilities ?\n> > > >\n> > > >\n> > > For example, on `ciri` with mtkisp7 pipeline handler [5], `pixelFormat`\n> > is\n> > > NV12,\n> > > supported resolution range is `320x240` to `2592x1944`, and\n> > `bufferCount` is\n> > > 8. It applies to `StillCapture`, `VideoRecording`, and `ViewFinder`.\n> > >\n> > > [5]:\n> > >\n> > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n> > >\n> >\n> > Looking at the pipeline handler implementation from your tree it seems\n> > to me that you're using roles in validate() to call setStream(), as we\n> > already clarified:\n> >\n> > switch (cfg.role) {\n> > case StreamRole::Viewfinder:\n> > case StreamRole::VideoRecording:\n> > if (videoCnt >= 2) {\n> > LOG(MtkISP7, Error)\n> > << \"Support only 2 Preview/Video\n> > streams\";\n> > return Invalid;\n> > }\n> > cfg.setStream(const_cast > *>(vidStreams[videoCnt++]));\n> > break;\n> > case StreamRole::StillCapture:\n> > if (stillCnt >= 2) {\n> > LOG(MtkISP7, Error)\n> > << \"Support only 2 StillCapture\n> > streams\";\n> > return Invalid;\n> > }\n> > cfg.setStream(const_cast > *>(stillStreams[stillCnt++]));\n> > break;\n> > default:\n> > LOG(MtkISP7, Error) << \"Invalid StreamRole \" <<\n> > cfg.role;\n> > return Invalid;\n> > }\n> >\n> > This shows that you have 4 streams, 2 for Preview/Video and 2 for\n> > StillCapture.\n> >\n> > Then you use the Stream in configure() for populate two sizes\n> >\n> > Size video1 = Size{ 0, 0 };\n> > Size video2 = Size{ 0, 0 };\n> > Size still1 = Size{ 0, 0 };\n> > Size still2 = Size{ 0, 0 };\n> >\n> > /* Only cover the video resolution */\n> > for (auto &cfg : *c) {\n> > if (cfg.stream() == &video1Stream_)\n> > video1 = cfg.size;\n> > else if (cfg.stream() == &video2Stream_)\n> > video2 = cfg.size;\n> > else if (cfg.stream() == &still1Stream_)\n> > still1 = cfg.size;\n> > else if (cfg.stream() == &still2Stream_)\n> > still2 = cfg.size;\n> > else\n> > return -EINVAL;\n> > }\n> >\n> > From there on, all the API you have implemented relies on the presence\n> > and order of the 'video1', 'video2, 'still1' and 'still2' parameters.\n> >\n> > In example\n> >\n> > mcnrManager.configure(camsysYuvSize, video1, video2);\n> > lpnrManager.configure(sensorFullSize_, still1, still2);\n> > lpnrTunManager.configure(sensorFullSize_, still1, still2);\n> > mcnrTunManager.configure(camsysYuvSize, video1, video2);\n> >\n> > Now, according to what I've read and what you said, there are no\n> > constraints on the HW that help identify Stream. To make an example,\n> > you can't assume \"Oh this StreamConfiguration is RGB so it needs to go\n> > to StreamX\".\n> >\n>\n> Yes, that's the main concern :)\n>\n>\n> >\n> > So I guess what you want is to allow an application to say \"One\n> > viewfinder is WxH, one Still capture is WxH, one viewfinder is WxH\"\n> > (also, all Streams are NV12 if I'm not mistaken).\n> >\n> > > As said, I'm not opposed to use Roles for assigning pipes, but I don't\n> > > > think we should based any new development on Roles as there's an high\n> > > > chance they can be reworked.\n> > > >\n> > >\n> > > Could you briefly describe how the new APIs would be like?\n> > >\n> > > Basically I think either applications need to have an argument to\n> > indicate\n> > > which kind of stream the StreamConfiguration needs to be assigned to, or\n> > > the pipeline handler needs to remember which StreamRole a\n> > > StreamConfiguration returned was asked as the default configuration for.\n> >\n> > It's not really about that, the idea is that the stream's\n> > characteristics should be used to assign a stream to a pipe, like the\n> > format and the sizes. In your case that's not really possible as all\n> > streams are NV12 and all resolutions can go anywhere.\n> >\n> > But, as you use roles, that logic should live somewhere, doesn't it ?\n> >\n> > Looking at your implementation of the Android HAL I see (sorry, cannot\n> > point out a commit id as there's quite some reverts in the history)\n> >\n> > if (isJpegStream(stream)) {\n> > continue;\n> > } else if (isYuvSnapshotStream(stream)) {\n> > streamConfig.streams = { { stream,\n> > CameraStream::Type::Direct } };\n> > streamConfig.config.role =\n> > StreamRole::StillCapture;\n> > } else if (isPreviewStream(stream)) {\n> > streamConfig.streams = { { stream,\n> > CameraStream::Type::Direct } };\n> > streamConfig.config.role = StreamRole::Viewfinder;\n> > } else if (isVideoStream(stream)) {\n> > streamConfig.streams = { { stream,\n> > CameraStream::Type::Direct } };\n> > streamConfig.config.role =\n> > StreamRole::VideoRecording;\n> > } else {\n> > streamConfig.streams = { { stream,\n> > CameraStream::Type::Direct } };\n> > streamConfig.config.role = StreamRole::Viewfinder;\n> > }\n> >\n> > So you populate roles based on some criteria here, and if I look at\n> > the criteria\n> >\n> > bool isPreviewStream(camera3_stream_t *stream)\n> > {\n> > return (GRALLOC_USAGE_HW_COMPOSER & stream->usage);\n> > }\n> >\n> > bool isVideoStream(camera3_stream_t *stream)\n> > {\n> > return (GRALLOC_USAGE_HW_VIDEO_ENCODER & stream->usage);\n> > }\n> >\n> > bool isYuvSnapshotStream(camera3_stream_t *stream)\n> > {\n> > return (!isVideoStream(stream) && !isPreviewStream(stream) &&\n> > (HAL_PIXEL_FORMAT_YCbCr_420_888 == stream->format));\n> > }\n> >\n> > they indeed come from Android's requirements that cannot be expressed\n> > in libcamera.\n> >\n> > To be clear, when asking \"what you need this for\" this is the level of\n> > detail that is needed to explain your design choices. Otherwise it's\n> > up to us to decode what you have done in the mtk support branch.\n> >\n>\n> I think that's also related to another confusion that I have regarding the\n> current libcamera API: We use `StreamRole` as the input to get the\n> default configuration, while it doesn't stop a pipeline handler to assign\n> it to a different stream, with a different StreamRole, later in `validate()`\n\nWhich makes me wonder, if we allow apps to set StreamConfig::role, how\nare we going to validate it ?\n\n> or `configure()`. The pipeline handler might not even return\n> `Status::Adjusted`, if the requested arguments are not changed.\n\nThe thing is that StreamRoles usage should have been limited to\ngenerateConfiguration() only. It shouldn't be related to which Stream\nin the pipeline handler a StreamConfig is assigned to, as there's no\n1-to-1 matching between StreamRoles (libcamera API) and the number and\ncharateristics of a Stream (platform specific).\n\n>\n> Maybe libcamera core libraries assume each ISP pipe has different\n> characteristics, like PixelFormat?\n>\n\nI presume so.\n\n>\n> >\n> >\n> > > It doesn't have to be `StreamRole`. If there are other types/arguments in\n> > > the new reworked API, I'd be happy to adapt to the new one(s).\n> > >\n> > > Two naive ideas:\n> > > 1. Keep the new `StreamRole role;` to be a const member variable in\n> > > StreamConfiguration, which should be set in\n> > > `PipelineHandler::generateConfiguration()`.\n> > >\n> > > 2. If it makes sense, add a private class for `StreamConfiguration` and\n> > keep\n> > > the new `StreamRole role;` there, so that applications cannot get the\n> > info.\n> > > (I don't think we need to hide the info from the applications though...)\n> > >\n> > > WDYT?\n> >\n> > Given the above described use case is my opinion valid, I wouldn't be\n> > opposed to have roles as a public member of the StreamConfiguration to\n> > allow application to hint how a stream should be used if no other\n> > criteria is applicable.\n> >\n>\n> Yeah I also think that the current patch is the simplest.\n>\n>\n> >\n> > >\n> > >\n> > > > Cc-Laurent for opinions.\n> >\n> > As this is an application-facing API change, I would however like to\n> > see more opinions.\n> >\n>\n> Sure, let's wait for more opinions from others :)\n>\n\nI'll make sure to discuss it with them in the next days\n\n>\n> >\n> > Thanks\n> > j\n> >\n> > > >\n> > > > >\n> > > > > BR,\n> > > > > Harvey\n> > > >\n> > >\n> > >\n> > > --\n> > > BR,\n> > > Harvey Yang\n> >\n>\n>\n> --\n> BR,\n> Harvey Yang","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 37E44C0F1B\n\tfor ;\n\tMon, 23 Sep 2024 15:10:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38AA96350B;\n\tMon, 23 Sep 2024 17:10:21 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED71A6037E\n\tfor ;\n\tMon, 23 Sep 2024 17:10:19 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 95F0163D;\n\tMon, 23 Sep 2024 17:08:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"k3bnAvo+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727104132;\n\tbh=/4pjPT3nSlcUoMAEQ3Ij2TFDoK6wlpbucH6hZ9bqvi0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=k3bnAvo+BA/ZgAYZYNMdyY4uw1n6EnakEITzwJ/UzotQafZ6J5b+58lwC2GzI5sat\n\t9Z1fCPwgm9XYkuzRvmPPHSx44AebIeBchLMDj0vp081F2UkXuw8saW7NKXCZGTW9Re\n\tghIsbkMKev673BJS74+UFN72j5fJMrYNz62Ld4Oc=","Date":"Mon, 23 Sep 2024 17:10:15 +0200","From":"Jacopo Mondi ","To":"Cheng-Hao Yang ","Cc":"Jacopo Mondi , \n\tLaurent Pinchart ,\n\tkieran bingham , \n\tCheng-Hao Yang ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","Message-ID":"","References":"<20240916045802.3799103-1-chenghaoyang@google.com>\n\t\n\t\n\t\n\t\n\t\n\t\n\t<5imcxykorjceprfzhynsijjian7p55fn5vry6ogfajsanczian@xtg6ooszbxo3>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31318,"web_url":"https://patchwork.libcamera.org/comment/31318/","msgid":"<875f9b54c80432b4d530ae821a7ad6db75b4499e.camel@ndufresne.ca>","date":"2024-09-23T18:36:16","subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Hi,\n\nLe lundi 23 septembre 2024 à 22:14 +0800, Cheng-Hao Yang a écrit :\n> Thanks Jacopo for looking into the tree of the mtkisp7 branch!\n\nSmall technical note, it will be appreciated by many users if you reply in text\nform. This is true for most mailing list out there.\n\nNicolas\n\n> \n> \n> On Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi wrote:\n> > Hi Harvey\n> > \n> > On Thu, Sep 19, 2024 at 09:55:58PM GMT, Cheng-Hao Yang wrote:\n> > > Hi Jacopo and Laurent,\n> > > \n> > > On Thu, Sep 19, 2024 at 5:09 PM Jacopo Mondi \n> > > wrote:\n> > > \n> > > > Hi Harvey\n> > > > \n> > > > On Wed, Sep 18, 2024 at 04:33:27PM GMT, Cheng-Hao Yang wrote:\n> > > > > Sorry Jacopo, my bad to miss the first message:\n> > > > > \n> > > > > On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi <\n> > > > jacopo.mondi@ideasonboard.com>\n> > > > > wrote:\n> > > > > \n> > > > > > Hi Harvey\n> > > > > > \n> > > > > > On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:\n> > > > > > > Hi Jacopo,\n> > > > > > > \n> > > > > > > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi <\n> > > > > > jacopo.mondi@ideasonboard.com>\n> > > > > > > wrote:\n> > > > > > > \n> > > > > > > > Hi Harvey\n> > > > > > > > \n> > > > > > > > On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:\n> > > > > > > > > Hi folks,\n> > > > > > > > > \n> > > > > > > > > Currently applications set resolutions, pixelFormat, bufferCount,\n> > > > > > etc,\n> > > > > > > > > into StreamConfigurations, and Pipeline Handler decides which\n> > > > streams\n> > > > > > > > > they're assigned to. However, it doesn't allow application to\n> > > > assign\n> > > > > > > > > streams that cannot be distinguished by those arguments into\n> > > > > > > > > VideoRecording or StillCapture (say YUV/NV12 format), which is\n> > > > > > needed in\n> > > > > > > > > mtkisp7.\n> > > > > > > > \n> > > > > > > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > > > > > > you plan to use StreamRole as part of the stream configuration ?\n> > > > > > > > \n> > > > > > \n> > > > > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > > > > you plan to use StreamRole as part of the stream configuration ?\n> > > > > > \n> > > > > > \n> > > > > > \n> > > > > In mtkisp7 pipeline handler, both preview (or video) and still capture\n> > > > > streams support the same format (NV12) and bufferCount, and the\n> > > > > maximum resolutions are also the same. Therefore, when calling\n> > > > > `validate()` and `configure()`, mtkisp7 pipeline handler doesn't know\n> > > > > if applications/Android adapter wants the stream to go through the\n> > > > > still captur pipeline or not.\n> > > > \n> > > > I still have an hard time understanding why validate() and configure()\n> > > > needs to know the \"role\". Is this to assign \"pipes\" to Streams ?\n> > > \n> > > \n> > > If I understand correctly that \"pipes\" means the pipelines of ISP/IPA algos\n> > > for preview/video/still capture respectively, yes, it's to assign the\n> > > StreamConfiguration(s) to the desired pipe(s), which means to call\n> > > `StreamConfiguration::setStream()`. According to the comments [1], it's\n> > > supposed to be called in `PipelineHandler::configure()`, like\n> > > SimplePipelineHandler [2]. However, there are also some pipeline handlers\n> > > that set them in `CameraConfiguration::validate()` [3] [4].\n> > \n> > Indeed most of our documentation suggests that setStream() is meant to\n> > be called during configure(). Considering that validate() is -always-\n> > called before configure() by Camera, so where exactly you call it, I\n> > don't think makes a big difference.\n> > \n> > We could also relax the documentation.\n> > \n> \n> \n> Yes, that's a great idea to avoid confusion.\n>  \n> > \n> > > \n> > > [1]:\n> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n303\n> > > [2]:\n> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/simple/simple.cpp#n1270\n> > > [3]:\n> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n347\n> > > [4]:\n> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rkisp1/rkisp1.cpp#n520\n> > > \n> > > Does\n> > > > the mtkisp7 pipes have different capabilities when it comes to\n> > > > supported formats and resolutions ?\n> > > > \n> > > \n> > > No, as you can see in mtkisp7's implementation [5], all of them support the\n> > > same formats and resolutions.\n> > > \n> > > [5]:\n> > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n> > > \n> > > \n> > > \n> > > \n> > > > > \n> > > > > Does that make sense :P?\n> > > > \n> > > > I guess so, but I wonder if the \"role\" is the main criteria that\n> > > > should be taken into account when assiging pipes to streams.\n> > > > \n> > > How would this work for applications that operates on platforms where\n> > > > the pipe selection depends on other criteria like the supported image\n> > > > formats and stream resolutions ? In example, for rkisp1\n> > > > the \"self\" pipelines can do RGB while the \"main\" can't and that's how\n> > > > we assign pipes during validate().\n> > > \n> > > \n> > > > To be honest this has not proven to be optimal and I'm not opposed to\n> > > > add Roles to the pipe selection criteria, but they shouldn't be made the\n> > > > only criteria on which pipes are assigned (unless we support this on\n> > > > all pipelines).\n> > > > \n> > > > \n> > > I agree that the `role` might not be the main criteria in general cases.\n> > > It's just that we find difficulties in assigning them properly with the\n> > > current\n> > > configurations in mtkisp7.\n> > > \n> > > \n> > > \n> > > > Also I would not based any future-proof design on Roles, they will\n> > > > likely be heavily reworked or removed going forward.\n> > > > \n> > > > As your main use case is Android, I think it would be doable for you\n> > > > to\n> > > > 1) Request a generateConfiguration() and keep track of the\n> > > >     StreamConfiguration order.\n> > > > \n> > > >     generateConfiguration(Viewfinder, Still, Raw)\n> > > > \n> > > >     will give you StreamConfiguration for the above rols in order as\n> > > >     you have requested them, if supported.\n> > > > \n> > > > 2) Code validate() so that you try to assing pipes based on the\n> > > >     formats/sizes and if formats/sizes are the same define an ordering.\n> > > >     In example the first YUV/WxH stream goes to Viewfinder if an\n> > > >     identical YUV/WxH stream is requested for Still Capture.\n> > > > \n> > > \n> > > This actually assumes the application doesn't change the content of the\n> > > default CameraConfiguration/StreamConfiguration, which I doubt if it's a\n> > > good thing. Currently in Android adapter, it re-arranges [6]\n> > > StreamConfigurations based on the previously-fetched default\n> > > StreamConfigurations of the StreamRoles, which I think is a normal\n> > > practice of the current interfaces' logic. Please correct me if I'm wrong :)\n> > \n> > Good point, as Android HAL re-sorts the StreamConfiguration before\n> > calling Camera::configure() you can't rely on the order in which you\n> > have requested roles to Camera::generateConfiguration().\n> > \n> > > \n> > > [6]:\n> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n708\n> > > \n> > > Also, if an application calls\n> > > `generateConfiguration(camera, {StreamRole::VideoRecording})` and\n> > > `generateConfiguration(camera, {StreamRole::StillCapture})`\n> > > respectively, and ends up calling `configure()` with `VideoRecording`'s\n> > > CameraConfiguration, how would the pipeline handler know that the\n> > > application intends to get buffers from the stream `VideoRecording`?\n> > > In your suggestion, it seems that the application needs to call\n> > > `generateConfiguration(camera, {StreamRole::VideoRecording})` again,\n> > > which I don't think is enforced in the current libcamera API, and might\n> > > not be a good practice.\n> > > \n> > > \n> > > > 3) As validate() assigns Steams * to StreamConfiguration (with\n> > > >    ::setStream) it should be easy to keep track to which pipe a\n> > > >    StreamConfiguration has been assigned to at configure() time.\n> > > > \n> > > > Could you list what are the platform's pipes capabilities ?\n> > > > \n> > > > \n> > > For example, on `ciri` with mtkisp7 pipeline handler [5], `pixelFormat` is\n> > > NV12,\n> > > supported resolution range is `320x240` to `2592x1944`, and `bufferCount` is\n> > > 8. It applies to `StillCapture`, `VideoRecording`, and `ViewFinder`.\n> > > \n> > > [5]:\n> > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n> > > \n> > \n> > Looking at the pipeline handler implementation from your tree it seems\n> > to me that you're using roles in validate() to call setStream(), as we\n> > already clarified:\n> > \n> >                 switch (cfg.role) {\n> >                 case StreamRole::Viewfinder:\n> >                 case StreamRole::VideoRecording:\n> >                         if (videoCnt >= 2) {\n> >                                 LOG(MtkISP7, Error)\n> >                                         << \"Support only 2 Preview/Video streams\";\n> >                                 return Invalid;\n> >                         }\n> >                         cfg.setStream(const_cast(vidStreams[videoCnt++]));\n> >                         break;\n> >                 case StreamRole::StillCapture:\n> >                         if (stillCnt >= 2) {\n> >                                 LOG(MtkISP7, Error)\n> >                                         << \"Support only 2 StillCapture streams\";\n> >                                 return Invalid;\n> >                         }\n> >                         cfg.setStream(const_cast(stillStreams[stillCnt++]));\n> >                         break;\n> >                 default:\n> >                         LOG(MtkISP7, Error) << \"Invalid StreamRole \" << cfg.role;\n> >                         return Invalid;\n> >                 }\n> > \n> > This shows that you have 4 streams, 2 for Preview/Video  and 2 for\n> > StillCapture.\n> > \n> > Then you use the Stream in configure() for populate two sizes\n> > \n> >         Size video1 = Size{ 0, 0 };\n> >         Size video2 = Size{ 0, 0 };\n> >         Size still1 = Size{ 0, 0 };\n> >         Size still2 = Size{ 0, 0 };\n> > \n> >         /* Only cover the video resolution */\n> >         for (auto &cfg : *c) {\n> >                 if (cfg.stream() == &video1Stream_)\n> >                         video1 = cfg.size;\n> >                 else if (cfg.stream() == &video2Stream_)\n> >                         video2 = cfg.size;\n> >                 else if (cfg.stream() == &still1Stream_)\n> >                         still1 = cfg.size;\n> >                 else if (cfg.stream() == &still2Stream_)\n> >                         still2 = cfg.size;\n> >                 else\n> >                         return -EINVAL;\n> >         }\n> > \n> > From there on, all the API you have implemented relies on the presence\n> > and order of the 'video1', 'video2, 'still1' and 'still2' parameters.\n> > \n> > In example\n> > \n> >         mcnrManager.configure(camsysYuvSize, video1, video2);\n> >         lpnrManager.configure(sensorFullSize_, still1, still2);\n> >         lpnrTunManager.configure(sensorFullSize_, still1, still2);\n> >         mcnrTunManager.configure(camsysYuvSize, video1, video2);\n> > \n> > Now, according to what I've read and what you said, there are no\n> > constraints on the HW that help identify Stream. To make an example,\n> > you can't assume \"Oh this StreamConfiguration is RGB so it needs to go\n> > to StreamX\".\n> > \n> \n> \n> Yes, that's the main concern :)\n>  \n> > \n> > So I guess what you want is to allow an application to say \"One\n> > viewfinder is WxH, one Still capture is WxH, one viewfinder is WxH\"\n> > (also, all Streams are NV12 if I'm not mistaken).\n> > \n> > > As said, I'm not opposed to use Roles for assigning pipes, but I don't\n> > > > think we should based any new development on Roles as there's an high\n> > > > chance they can be reworked.\n> > > > \n> > > \n> > > Could you briefly describe how the new APIs would be like?\n> > > \n> > > Basically I think either applications need to have an argument to indicate\n> > > which kind of stream the StreamConfiguration needs to be assigned to, or\n> > > the pipeline handler needs to remember which StreamRole a\n> > > StreamConfiguration returned was asked as the default configuration for.\n> > \n> > It's not really about that, the idea is that the stream's\n> > characteristics should be used to assign a stream to a pipe, like the\n> > format and the sizes. In your case that's not really possible as all\n> > streams are NV12 and all resolutions can go anywhere.\n> > \n> > But, as you use roles, that logic should live somewhere, doesn't it ?\n> > \n> > Looking at your implementation of the Android HAL I see (sorry, cannot\n> > point out a commit id as there's quite some reverts in the history)\n> > \n> >                 if (isJpegStream(stream)) {\n> >                         continue;\n> >                 } else if (isYuvSnapshotStream(stream)) {\n> >                         streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> >                         streamConfig.config.role = StreamRole::StillCapture;\n> >                 } else if (isPreviewStream(stream)) {\n> >                         streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> >                         streamConfig.config.role = StreamRole::Viewfinder;\n> >                 } else if (isVideoStream(stream)) {\n> >                         streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> >                         streamConfig.config.role = StreamRole::VideoRecording;\n> >                 } else {\n> >                         streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> >                         streamConfig.config.role = StreamRole::Viewfinder;\n> >                 }\n> > \n> > So you populate roles based on some criteria here, and if I look at\n> > the criteria\n> > \n> > bool isPreviewStream(camera3_stream_t *stream)\n> > {\n> >         return (GRALLOC_USAGE_HW_COMPOSER & stream->usage);\n> > }\n> > \n> > bool isVideoStream(camera3_stream_t *stream)\n> > {\n> >         return (GRALLOC_USAGE_HW_VIDEO_ENCODER & stream->usage);\n> > }\n> > \n> > bool isYuvSnapshotStream(camera3_stream_t *stream)\n> > {\n> >         return (!isVideoStream(stream) && !isPreviewStream(stream) &&\n> >                 (HAL_PIXEL_FORMAT_YCbCr_420_888 == stream->format));\n> > }\n> > \n> > they indeed come from Android's requirements that cannot be expressed\n> > in libcamera.\n> > \n> > To be clear, when asking \"what you need this for\" this is the level of\n> > detail that is needed to explain your design choices. Otherwise it's\n> > up to us to decode what you have done in the mtk support branch.\n> > \n> \n> \n> I think that's also related to another confusion that I have regarding the\n> current libcamera API: We use `StreamRole` as the input to get the\n> default configuration, while it doesn't stop a pipeline handler to assign\n> it to a different stream, with a different StreamRole, later in `validate()`\n> or `configure()`. The pipeline handler might not even return\n> `Status::Adjusted`, if the requested arguments are not changed.\n> \n> Maybe libcamera core libraries assume each ISP pipe has different\n> characteristics, like PixelFormat?\n>  \n> > \n> > \n> > > It doesn't have to be `StreamRole`. If there are other types/arguments in\n> > > the new reworked API, I'd be happy to adapt to the new one(s).\n> > > \n> > > Two naive ideas:\n> > > 1. Keep the new `StreamRole role;` to be a const member variable in\n> > > StreamConfiguration, which should be set in\n> > > `PipelineHandler::generateConfiguration()`.\n> > > \n> > > 2. If it makes sense, add a private class for `StreamConfiguration` and keep\n> > > the new `StreamRole role;` there, so that applications cannot get the info.\n> > > (I don't think we need to hide the info from the applications though...)\n> > > \n> > > WDYT?\n> > \n> > Given the above described use case is my opinion valid, I wouldn't be\n> > opposed to have roles as a public member of the StreamConfiguration to\n> > allow application to hint how a stream should be used if no other\n> > criteria is applicable.\n> > \n> \n> \n> Yeah I also think that the current patch is the simplest.\n>  \n> > \n> > > \n> > > \n> > > > Cc-Laurent for opinions.\n> > \n> > As this is an application-facing API change, I would however like to\n> > see more opinions.\n> > \n> \n> \n> Sure, let's wait for more opinions from others :)\n>  \n> > \n> > Thanks\n> >   j\n> > \n> > > > \n> > > > > \n> > > > > BR,\n> > > > > Harvey\n> > > > \n> > > \n> > > \n> > > --\n> > > BR,\n> > > Harvey Yang\n> \n> \n> -- \n> BR,\n> Harvey Yang","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 B00FCC3257\n\tfor ;\n\tMon, 23 Sep 2024 18:36:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B3E3618DE;\n\tMon, 23 Sep 2024 20:36:22 +0200 (CEST)","from mail-qv1-xf32.google.com (mail-qv1-xf32.google.com\n\t[IPv6:2607:f8b0:4864:20::f32])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7F0E66037E\n\tfor ;\n\tMon, 23 Sep 2024 20:36:19 +0200 (CEST)","by mail-qv1-xf32.google.com with SMTP id\n\t6a1803df08f44-6c3561804b5so39711236d6.0\n\tfor ;\n\tMon, 23 Sep 2024 11:36:19 -0700 (PDT)","from nicolas-tpx395.lan ([2606:6d00:15:862e::580])\n\tby smtp.gmail.com with ESMTPSA id\n\t6a1803df08f44-6c75e57b00bsm50128376d6.118.2024.09.23.11.36.17\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 23 Sep 2024 11:36:17 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ndufresne-ca.20230601.gappssmtp.com\n\theader.i=@ndufresne-ca.20230601.gappssmtp.com\n\theader.b=\"jMWgO72W\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20230601.gappssmtp.com; s=20230601; t=1727116578;\n\tx=1727721378; darn=lists.libcamera.org; \n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:from:to:cc:subject\n\t:date:message-id:reply-to;\n\tbh=CiqT3yuqwZ8UQak9mMzxii4pLQp6xf76AW5mlsEh2GA=;\n\tb=jMWgO72Wq2ZJYhSsYDUgsOROkyYQZT4aFs2XkrH+QswFb08S+A9Pjcsw9zTxG/RPeB\n\t2BfyzvSQRCK7gHE9Fc9VDx6xWLE/VqRN9KtVdIoHg7d09YgjLW52xVCSzZA593VwB0ez\n\tSnBnVEli9Vy6f3dVLWGlf/NKHsVHYmt7or+98dV0j+qjsoirrgnISddQSbz/3WWyAZT6\n\tKBR13RkQC4qJRQKCMbT/Wdo2M9GvXzC5xwdRN1gJvwtj+azSSHf5DVU9LYaYGszU4qpG\n\tlv03dC9xq3M9uXJ+4dZQ8Q5oQJt6rpKDUbjEKnyn49yTX0wPimPk1FWpfNApIrv3YOBi\n\tsq5w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1727116578; x=1727721378;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=CiqT3yuqwZ8UQak9mMzxii4pLQp6xf76AW5mlsEh2GA=;\n\tb=LUxyD8246nGpGmPWZVla+BZ4JOM2KgCzfOrLeiwr/t8y6K6/XyulnXnvh2lg402z5s\n\tr8NYc4uf51GvmHC0yIji5xZq45cJVRgAbXyyCXcB2ss8X8paFvz72mjtUMtAzDT94aVN\n\tghSOa6eQgc1DU9x1MKAVXDvoWL9kntM5h2kA7zrgVFM8DY74PH/haehVlqIic3BKezWD\n\t0DY4BJgC7HBsRfSuFC+G0OdQAey45nckMCKJGAOv832+BSM12E4PIqB2XF628qmrgLix\n\tZ4nCwFnb3uYxZVtuAk6lIgfVS3jGEvzx/hCNaOf+DPIJf8dCUBHVWBHAjtquKOlRNn30\n\t1+Hg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXdIFKF0/4bSRJ1c3yYBU4m0Uu9n1K8JMZg8NlLguM8EFhWwZeeitY80crJ3GI5gp+Os7d2JsrnIXornYdmVhY=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Ywh36e6V0ehu6jM14zyNFdpDt6LWe3RKQHwSCSib/eJrhP2Hja3\n\tBsHMLX6Y1Eo/RXCVfgNV1QD407D85ngdwV2al532QzVmZIWugbeF9PvPs0mgAPk=","X-Google-Smtp-Source":"AGHT+IGZLfMZqTG3zoouddoPGqpqDiWKwyUTGhEpdtkfpdYNbDx0vE1shy9nAIt5ie8S3uF2isdoig==","X-Received":"by 2002:a05:6214:5909:b0:6c5:a440:f43 with SMTP id\n\t6a1803df08f44-6c7bc67b3f7mr192032106d6.8.1727116578091; \n\tMon, 23 Sep 2024 11:36:18 -0700 (PDT)","Message-ID":"<875f9b54c80432b4d530ae821a7ad6db75b4499e.camel@ndufresne.ca>","Subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","From":"Nicolas Dufresne ","To":"Cheng-Hao Yang , Jacopo Mondi\n\t","Cc":"Laurent Pinchart , kieran bingham\n\t, Cheng-Hao Yang\n\t, libcamera-devel@lists.libcamera.org","Date":"Mon, 23 Sep 2024 14:36:16 -0400","In-Reply-To":"","References":"<20240916045802.3799103-1-chenghaoyang@google.com>\n\t\n\t\n\t\n\t\n\t\n\t\n\t<5imcxykorjceprfzhynsijjian7p55fn5vry6ogfajsanczian@xtg6ooszbxo3>\n\t","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.52.4 (3.52.4-1.fc40) ","MIME-Version":"1.0","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31327,"web_url":"https://patchwork.libcamera.org/comment/31327/","msgid":"","date":"2024-09-24T05:29:16","subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"On Tue, Sep 24, 2024 at 2:36 AM Nicolas Dufresne wrote:\n>\n>\n> Hi,\n>\n> Le lundi 23 septembre 2024 à 22:14 +0800, Cheng-Hao Yang a écrit :\n> > Thanks Jacopo for looking into the tree of the mtkisp7 branch!\n>\n> Small technical note, it will be appreciated by many users if you reply in text\n> form. This is true for most mailing list out there.\n\nAh okay. Hope I'm doing it right this way.\n\n\n>\n>\n> Nicolas\n>\n> >\n> >\n> > On Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi wrote:\n> > > Hi Harvey\n> > >\n> > > On Thu, Sep 19, 2024 at 09:55:58PM GMT, Cheng-Hao Yang wrote:\n> > > > Hi Jacopo and Laurent,\n> > > >\n> > > > On Thu, Sep 19, 2024 at 5:09 PM Jacopo Mondi \n> > > > wrote:\n> > > >\n> > > > > Hi Harvey\n> > > > >\n> > > > > On Wed, Sep 18, 2024 at 04:33:27PM GMT, Cheng-Hao Yang wrote:\n> > > > > > Sorry Jacopo, my bad to miss the first message:\n> > > > > >\n> > > > > > On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi <\n> > > > > jacopo.mondi@ideasonboard.com>\n> > > > > > wrote:\n> > > > > >\n> > > > > > > Hi Harvey\n> > > > > > >\n> > > > > > > On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:\n> > > > > > > > Hi Jacopo,\n> > > > > > > >\n> > > > > > > > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi <\n> > > > > > > jacopo.mondi@ideasonboard.com>\n> > > > > > > > wrote:\n> > > > > > > >\n> > > > > > > > > Hi Harvey\n> > > > > > > > >\n> > > > > > > > > On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:\n> > > > > > > > > > Hi folks,\n> > > > > > > > > >\n> > > > > > > > > > Currently applications set resolutions, pixelFormat, bufferCount,\n> > > > > > > etc,\n> > > > > > > > > > into StreamConfigurations, and Pipeline Handler decides which\n> > > > > streams\n> > > > > > > > > > they're assigned to. However, it doesn't allow application to\n> > > > > assign\n> > > > > > > > > > streams that cannot be distinguished by those arguments into\n> > > > > > > > > > VideoRecording or StillCapture (say YUV/NV12 format), which is\n> > > > > > > needed in\n> > > > > > > > > > mtkisp7.\n> > > > > > > > >\n> > > > > > > > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > > > > > > > you plan to use StreamRole as part of the stream configuration ?\n> > > > > > > > >\n> > > > > > >\n> > > > > > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > > > > > you plan to use StreamRole as part of the stream configuration ?\n> > > > > > >\n> > > > > > >\n> > > > > > >\n> > > > > > In mtkisp7 pipeline handler, both preview (or video) and still capture\n> > > > > > streams support the same format (NV12) and bufferCount, and the\n> > > > > > maximum resolutions are also the same. Therefore, when calling\n> > > > > > `validate()` and `configure()`, mtkisp7 pipeline handler doesn't know\n> > > > > > if applications/Android adapter wants the stream to go through the\n> > > > > > still captur pipeline or not.\n> > > > >\n> > > > > I still have an hard time understanding why validate() and configure()\n> > > > > needs to know the \"role\". Is this to assign \"pipes\" to Streams ?\n> > > >\n> > > >\n> > > > If I understand correctly that \"pipes\" means the pipelines of ISP/IPA algos\n> > > > for preview/video/still capture respectively, yes, it's to assign the\n> > > > StreamConfiguration(s) to the desired pipe(s), which means to call\n> > > > `StreamConfiguration::setStream()`. According to the comments [1], it's\n> > > > supposed to be called in `PipelineHandler::configure()`, like\n> > > > SimplePipelineHandler [2]. However, there are also some pipeline handlers\n> > > > that set them in `CameraConfiguration::validate()` [3] [4].\n> > >\n> > > Indeed most of our documentation suggests that setStream() is meant to\n> > > be called during configure(). Considering that validate() is -always-\n> > > called before configure() by Camera, so where exactly you call it, I\n> > > don't think makes a big difference.\n> > >\n> > > We could also relax the documentation.\n> > >\n> >\n> >\n> > Yes, that's a great idea to avoid confusion.\n> >\n> > >\n> > > >\n> > > > [1]:\n> > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n303\n> > > > [2]:\n> > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/simple/simple.cpp#n1270\n> > > > [3]:\n> > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n347\n> > > > [4]:\n> > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rkisp1/rkisp1.cpp#n520\n> > > >\n> > > > Does\n> > > > > the mtkisp7 pipes have different capabilities when it comes to\n> > > > > supported formats and resolutions ?\n> > > > >\n> > > >\n> > > > No, as you can see in mtkisp7's implementation [5], all of them support the\n> > > > same formats and resolutions.\n> > > >\n> > > > [5]:\n> > > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n> > > >\n> > > >\n> > > >\n> > > >\n> > > > > >\n> > > > > > Does that make sense :P?\n> > > > >\n> > > > > I guess so, but I wonder if the \"role\" is the main criteria that\n> > > > > should be taken into account when assiging pipes to streams.\n> > > > >\n> > > > How would this work for applications that operates on platforms where\n> > > > > the pipe selection depends on other criteria like the supported image\n> > > > > formats and stream resolutions ? In example, for rkisp1\n> > > > > the \"self\" pipelines can do RGB while the \"main\" can't and that's how\n> > > > > we assign pipes during validate().\n> > > >\n> > > >\n> > > > > To be honest this has not proven to be optimal and I'm not opposed to\n> > > > > add Roles to the pipe selection criteria, but they shouldn't be made the\n> > > > > only criteria on which pipes are assigned (unless we support this on\n> > > > > all pipelines).\n> > > > >\n> > > > >\n> > > > I agree that the `role` might not be the main criteria in general cases.\n> > > > It's just that we find difficulties in assigning them properly with the\n> > > > current\n> > > > configurations in mtkisp7.\n> > > >\n> > > >\n> > > >\n> > > > > Also I would not based any future-proof design on Roles, they will\n> > > > > likely be heavily reworked or removed going forward.\n> > > > >\n> > > > > As your main use case is Android, I think it would be doable for you\n> > > > > to\n> > > > > 1) Request a generateConfiguration() and keep track of the\n> > > > > StreamConfiguration order.\n> > > > >\n> > > > > generateConfiguration(Viewfinder, Still, Raw)\n> > > > >\n> > > > > will give you StreamConfiguration for the above rols in order as\n> > > > > you have requested them, if supported.\n> > > > >\n> > > > > 2) Code validate() so that you try to assing pipes based on the\n> > > > > formats/sizes and if formats/sizes are the same define an ordering.\n> > > > > In example the first YUV/WxH stream goes to Viewfinder if an\n> > > > > identical YUV/WxH stream is requested for Still Capture.\n> > > > >\n> > > >\n> > > > This actually assumes the application doesn't change the content of the\n> > > > default CameraConfiguration/StreamConfiguration, which I doubt if it's a\n> > > > good thing. Currently in Android adapter, it re-arranges [6]\n> > > > StreamConfigurations based on the previously-fetched default\n> > > > StreamConfigurations of the StreamRoles, which I think is a normal\n> > > > practice of the current interfaces' logic. Please correct me if I'm wrong :)\n> > >\n> > > Good point, as Android HAL re-sorts the StreamConfiguration before\n> > > calling Camera::configure() you can't rely on the order in which you\n> > > have requested roles to Camera::generateConfiguration().\n> > >\n> > > >\n> > > > [6]:\n> > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n708\n> > > >\n> > > > Also, if an application calls\n> > > > `generateConfiguration(camera, {StreamRole::VideoRecording})` and\n> > > > `generateConfiguration(camera, {StreamRole::StillCapture})`\n> > > > respectively, and ends up calling `configure()` with `VideoRecording`'s\n> > > > CameraConfiguration, how would the pipeline handler know that the\n> > > > application intends to get buffers from the stream `VideoRecording`?\n> > > > In your suggestion, it seems that the application needs to call\n> > > > `generateConfiguration(camera, {StreamRole::VideoRecording})` again,\n> > > > which I don't think is enforced in the current libcamera API, and might\n> > > > not be a good practice.\n> > > >\n> > > >\n> > > > > 3) As validate() assigns Steams * to StreamConfiguration (with\n> > > > > ::setStream) it should be easy to keep track to which pipe a\n> > > > > StreamConfiguration has been assigned to at configure() time.\n> > > > >\n> > > > > Could you list what are the platform's pipes capabilities ?\n> > > > >\n> > > > >\n> > > > For example, on `ciri` with mtkisp7 pipeline handler [5], `pixelFormat` is\n> > > > NV12,\n> > > > supported resolution range is `320x240` to `2592x1944`, and `bufferCount` is\n> > > > 8. It applies to `StillCapture`, `VideoRecording`, and `ViewFinder`.\n> > > >\n> > > > [5]:\n> > > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n> > > >\n> > >\n> > > Looking at the pipeline handler implementation from your tree it seems\n> > > to me that you're using roles in validate() to call setStream(), as we\n> > > already clarified:\n> > >\n> > > switch (cfg.role) {\n> > > case StreamRole::Viewfinder:\n> > > case StreamRole::VideoRecording:\n> > > if (videoCnt >= 2) {\n> > > LOG(MtkISP7, Error)\n> > > << \"Support only 2 Preview/Video streams\";\n> > > return Invalid;\n> > > }\n> > > cfg.setStream(const_cast(vidStreams[videoCnt++]));\n> > > break;\n> > > case StreamRole::StillCapture:\n> > > if (stillCnt >= 2) {\n> > > LOG(MtkISP7, Error)\n> > > << \"Support only 2 StillCapture streams\";\n> > > return Invalid;\n> > > }\n> > > cfg.setStream(const_cast(stillStreams[stillCnt++]));\n> > > break;\n> > > default:\n> > > LOG(MtkISP7, Error) << \"Invalid StreamRole \" << cfg.role;\n> > > return Invalid;\n> > > }\n> > >\n> > > This shows that you have 4 streams, 2 for Preview/Video and 2 for\n> > > StillCapture.\n> > >\n> > > Then you use the Stream in configure() for populate two sizes\n> > >\n> > > Size video1 = Size{ 0, 0 };\n> > > Size video2 = Size{ 0, 0 };\n> > > Size still1 = Size{ 0, 0 };\n> > > Size still2 = Size{ 0, 0 };\n> > >\n> > > /* Only cover the video resolution */\n> > > for (auto &cfg : *c) {\n> > > if (cfg.stream() == &video1Stream_)\n> > > video1 = cfg.size;\n> > > else if (cfg.stream() == &video2Stream_)\n> > > video2 = cfg.size;\n> > > else if (cfg.stream() == &still1Stream_)\n> > > still1 = cfg.size;\n> > > else if (cfg.stream() == &still2Stream_)\n> > > still2 = cfg.size;\n> > > else\n> > > return -EINVAL;\n> > > }\n> > >\n> > > From there on, all the API you have implemented relies on the presence\n> > > and order of the 'video1', 'video2, 'still1' and 'still2' parameters.\n> > >\n> > > In example\n> > >\n> > > mcnrManager.configure(camsysYuvSize, video1, video2);\n> > > lpnrManager.configure(sensorFullSize_, still1, still2);\n> > > lpnrTunManager.configure(sensorFullSize_, still1, still2);\n> > > mcnrTunManager.configure(camsysYuvSize, video1, video2);\n> > >\n> > > Now, according to what I've read and what you said, there are no\n> > > constraints on the HW that help identify Stream. To make an example,\n> > > you can't assume \"Oh this StreamConfiguration is RGB so it needs to go\n> > > to StreamX\".\n> > >\n> >\n> >\n> > Yes, that's the main concern :)\n> >\n> > >\n> > > So I guess what you want is to allow an application to say \"One\n> > > viewfinder is WxH, one Still capture is WxH, one viewfinder is WxH\"\n> > > (also, all Streams are NV12 if I'm not mistaken).\n> > >\n> > > > As said, I'm not opposed to use Roles for assigning pipes, but I don't\n> > > > > think we should based any new development on Roles as there's an high\n> > > > > chance they can be reworked.\n> > > > >\n> > > >\n> > > > Could you briefly describe how the new APIs would be like?\n> > > >\n> > > > Basically I think either applications need to have an argument to indicate\n> > > > which kind of stream the StreamConfiguration needs to be assigned to, or\n> > > > the pipeline handler needs to remember which StreamRole a\n> > > > StreamConfiguration returned was asked as the default configuration for.\n> > >\n> > > It's not really about that, the idea is that the stream's\n> > > characteristics should be used to assign a stream to a pipe, like the\n> > > format and the sizes. In your case that's not really possible as all\n> > > streams are NV12 and all resolutions can go anywhere.\n> > >\n> > > But, as you use roles, that logic should live somewhere, doesn't it ?\n> > >\n> > > Looking at your implementation of the Android HAL I see (sorry, cannot\n> > > point out a commit id as there's quite some reverts in the history)\n> > >\n> > > if (isJpegStream(stream)) {\n> > > continue;\n> > > } else if (isYuvSnapshotStream(stream)) {\n> > > streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> > > streamConfig.config.role = StreamRole::StillCapture;\n> > > } else if (isPreviewStream(stream)) {\n> > > streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> > > streamConfig.config.role = StreamRole::Viewfinder;\n> > > } else if (isVideoStream(stream)) {\n> > > streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> > > streamConfig.config.role = StreamRole::VideoRecording;\n> > > } else {\n> > > streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> > > streamConfig.config.role = StreamRole::Viewfinder;\n> > > }\n> > >\n> > > So you populate roles based on some criteria here, and if I look at\n> > > the criteria\n> > >\n> > > bool isPreviewStream(camera3_stream_t *stream)\n> > > {\n> > > return (GRALLOC_USAGE_HW_COMPOSER & stream->usage);\n> > > }\n> > >\n> > > bool isVideoStream(camera3_stream_t *stream)\n> > > {\n> > > return (GRALLOC_USAGE_HW_VIDEO_ENCODER & stream->usage);\n> > > }\n> > >\n> > > bool isYuvSnapshotStream(camera3_stream_t *stream)\n> > > {\n> > > return (!isVideoStream(stream) && !isPreviewStream(stream) &&\n> > > (HAL_PIXEL_FORMAT_YCbCr_420_888 == stream->format));\n> > > }\n> > >\n> > > they indeed come from Android's requirements that cannot be expressed\n> > > in libcamera.\n> > >\n> > > To be clear, when asking \"what you need this for\" this is the level of\n> > > detail that is needed to explain your design choices. Otherwise it's\n> > > up to us to decode what you have done in the mtk support branch.\n> > >\n> >\n> >\n> > I think that's also related to another confusion that I have regarding the\n> > current libcamera API: We use `StreamRole` as the input to get the\n> > default configuration, while it doesn't stop a pipeline handler to assign\n> > it to a different stream, with a different StreamRole, later in `validate()`\n> > or `configure()`. The pipeline handler might not even return\n> > `Status::Adjusted`, if the requested arguments are not changed.\n> >\n> > Maybe libcamera core libraries assume each ISP pipe has different\n> > characteristics, like PixelFormat?\n> >\n> > >\n> > >\n> > > > It doesn't have to be `StreamRole`. If there are other types/arguments in\n> > > > the new reworked API, I'd be happy to adapt to the new one(s).\n> > > >\n> > > > Two naive ideas:\n> > > > 1. Keep the new `StreamRole role;` to be a const member variable in\n> > > > StreamConfiguration, which should be set in\n> > > > `PipelineHandler::generateConfiguration()`.\n> > > >\n> > > > 2. If it makes sense, add a private class for `StreamConfiguration` and keep\n> > > > the new `StreamRole role;` there, so that applications cannot get the info.\n> > > > (I don't think we need to hide the info from the applications though...)\n> > > >\n> > > > WDYT?\n> > >\n> > > Given the above described use case is my opinion valid, I wouldn't be\n> > > opposed to have roles as a public member of the StreamConfiguration to\n> > > allow application to hint how a stream should be used if no other\n> > > criteria is applicable.\n> > >\n> >\n> >\n> > Yeah I also think that the current patch is the simplest.\n> >\n> > >\n> > > >\n> > > >\n> > > > > Cc-Laurent for opinions.\n> > >\n> > > As this is an application-facing API change, I would however like to\n> > > see more opinions.\n> > >\n> >\n> >\n> > Sure, let's wait for more opinions from others :)\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > > >\n> > > > > >\n> > > > > > BR,\n> > > > > > Harvey\n> > > > >\n> > > >\n> > > >\n> > > > --\n> > > > BR,\n> > > > Harvey Yang\n> >\n> >\n> > --\n> > BR,\n> > Harvey Yang\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 428E8C3257\n\tfor ;\n\tTue, 24 Sep 2024 05:29:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 468AB634F2;\n\tTue, 24 Sep 2024 07:29:31 +0200 (CEST)","from mail-lj1-x231.google.com (mail-lj1-x231.google.com\n\t[IPv6:2a00:1450:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C312E618DA\n\tfor ;\n\tTue, 24 Sep 2024 07:29:28 +0200 (CEST)","by mail-lj1-x231.google.com with SMTP id\n\t38308e7fff4ca-2f75d044201so49888731fa.0\n\tfor ;\n\tMon, 23 Sep 2024 22:29:28 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"h5/Ze0tG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1727155768; x=1727760568;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=9PIf3jpMmh3XsLZafgLM+aY4YazxS7YM//fTDkDYfQc=;\n\tb=h5/Ze0tG6QDt7Sht2fR7ZLcqKQt7hMTyHdCijURtgTlqyUfGvTh0lPCam10bjjHp+H\n\tNB/Soo4965Vd++QBdgPnkrUIzax0Mgh0os0TSlzV/8N9qNexnUIHcDsFbGtMsAU7m92h\n\t4uXwwaMnxLzK8Que0CYyJcpM3poe4M2ihek4o=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1727155768; x=1727760568;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=9PIf3jpMmh3XsLZafgLM+aY4YazxS7YM//fTDkDYfQc=;\n\tb=LcBBsO28LmFk0yeQcULMPRdtlxj05GGsZCAXF9Gj7iOmFRnF2Vd4UvXYaA6W4+vV8i\n\teyQ4nf23h3cl/mF9MRUBKnBTjsojPIsUBVvA6xzQYSyNnVrab4GCUfWfRJg1jh+vENwc\n\tcVQqR04WtavMcNpGzT64EEVeD1IpJ3B0iROg18iuifAZ2GSjEe3rpde9ROycYDz7fWXe\n\tUiF3jCL7/XpQlYh7Z54axljwhHldrd5B7zB409STpA35FAgKlHxPSTxiYTWjmuV+xoAd\n\tgbj2P3DuUj8ikEfWG6K3ain7UfKqePk/qXJGpAd1DpxRmvpc8TEZeNt6IbxwKtqgSRZi\n\tRoXg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVHnSlWpB4OdAvVTuH7URWdB2Hrg0fVFMPfkc66TtorQAt59H7xGB4wzeAj93meMEta5izUE5jg+DUHhIj8gUA=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YztUkPE7Xf6Pjbnl2D7uRY/DVfa5kjywBe++07poV0xZ66jF5jj\n\tdxiaV45O4HPP4B4eETnKO5kHd068Qkvhu7foO4f8gn8UvawGGZ4Vc7Iw5xvxMjFKto24QlLjSvg\n\tJgUYEqt8LEIkFkkQ/sTSjpiBA4iwSYV+VIEF8","X-Google-Smtp-Source":"AGHT+IEW8yRbnVqQs7WnbuSVL6lYRbm2DhymZIN3gI5qWNPWuShcbnOTx0hAVHwwhECSao+DPJVZFx+QPSpksCgMico=","X-Received":"by 2002:a05:651c:19a3:b0:2ef:2c40:dd67 with SMTP id\n\t38308e7fff4ca-2f8d0b61b89mr6204611fa.3.1727155767475; Mon, 23 Sep 2024\n\t22:29:27 -0700 (PDT)","MIME-Version":"1.0","References":"<20240916045802.3799103-1-chenghaoyang@google.com>\n\t\n\t\n\t\n\t\n\t\n\t\n\t<5imcxykorjceprfzhynsijjian7p55fn5vry6ogfajsanczian@xtg6ooszbxo3>\n\t\n\t<875f9b54c80432b4d530ae821a7ad6db75b4499e.camel@ndufresne.ca>","In-Reply-To":"<875f9b54c80432b4d530ae821a7ad6db75b4499e.camel@ndufresne.ca>","From":"Cheng-Hao Yang ","Date":"Tue, 24 Sep 2024 13:29:16 +0800","Message-ID":"","Subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","To":"Nicolas Dufresne ","Cc":"Cheng-Hao Yang ,\n\tJacopo Mondi , \n\tLaurent Pinchart , \n\tkieran bingham ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31329,"web_url":"https://patchwork.libcamera.org/comment/31329/","msgid":"","date":"2024-09-24T06:42:43","subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Jacopo,\n\nOn Mon, Sep 23, 2024 at 11:10 PM Jacopo Mondi\n wrote:\n>\n> Hi Harvey\n>\n> On Mon, Sep 23, 2024 at 10:14:48PM GMT, Cheng-Hao Yang wrote:\n> > Thanks Jacopo for looking into the tree of the mtkisp7 branch!\n> >\n> >\n> > On Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi \n> > wrote:\n> >\n> > > Hi Harvey\n> > >\n> > > On Thu, Sep 19, 2024 at 09:55:58PM GMT, Cheng-Hao Yang wrote:\n> > > > Hi Jacopo and Laurent,\n> > > >\n> > > > On Thu, Sep 19, 2024 at 5:09 PM Jacopo Mondi <\n> > > jacopo.mondi@ideasonboard.com>\n> > > > wrote:\n> > > >\n> > > > > Hi Harvey\n> > > > >\n> > > > > On Wed, Sep 18, 2024 at 04:33:27PM GMT, Cheng-Hao Yang wrote:\n> > > > > > Sorry Jacopo, my bad to miss the first message:\n> > > > > >\n> > > > > > On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi <\n> > > > > jacopo.mondi@ideasonboard.com>\n> > > > > > wrote:\n> > > > > >\n> > > > > > > Hi Harvey\n> > > > > > >\n> > > > > > > On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:\n> > > > > > > > Hi Jacopo,\n> > > > > > > >\n> > > > > > > > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi <\n> > > > > > > jacopo.mondi@ideasonboard.com>\n> > > > > > > > wrote:\n> > > > > > > >\n> > > > > > > > > Hi Harvey\n> > > > > > > > >\n> > > > > > > > > On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:\n> > > > > > > > > > Hi folks,\n> > > > > > > > > >\n> > > > > > > > > > Currently applications set resolutions, pixelFormat,\n> > > bufferCount,\n> > > > > > > etc,\n> > > > > > > > > > into StreamConfigurations, and Pipeline Handler decides which\n> > > > > streams\n> > > > > > > > > > they're assigned to. However, it doesn't allow application to\n> > > > > assign\n> > > > > > > > > > streams that cannot be distinguished by those arguments into\n> > > > > > > > > > VideoRecording or StillCapture (say YUV/NV12 format), which\n> > > is\n> > > > > > > needed in\n> > > > > > > > > > mtkisp7.\n> > > > > > > > >\n> > > > > > > > > Could you explain in a bit more detail why this \"is needed\"\n> > > and how\n> > > > > > > > > you plan to use StreamRole as part of the stream configuration\n> > > ?\n> > > > > > > > >\n> > > > > > >\n> > > > > > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > > > > > you plan to use StreamRole as part of the stream configuration ?\n> > > > > > >\n> > > > > > >\n> > > > > > >\n> > > > > > In mtkisp7 pipeline handler, both preview (or video) and still\n> > > capture\n> > > > > > streams support the same format (NV12) and bufferCount, and the\n> > > > > > maximum resolutions are also the same. Therefore, when calling\n> > > > > > `validate()` and `configure()`, mtkisp7 pipeline handler doesn't know\n> > > > > > if applications/Android adapter wants the stream to go through the\n> > > > > > still captur pipeline or not.\n> > > > >\n> > > > > I still have an hard time understanding why validate() and configure()\n> > > > > needs to know the \"role\". Is this to assign \"pipes\" to Streams ?\n> > > >\n> > > >\n> > > > If I understand correctly that \"pipes\" means the pipelines of ISP/IPA\n> > > algos\n> > > > for preview/video/still capture respectively, yes, it's to assign the\n> > > > StreamConfiguration(s) to the desired pipe(s), which means to call\n> > > > `StreamConfiguration::setStream()`. According to the comments [1], it's\n> > > > supposed to be called in `PipelineHandler::configure()`, like\n> > > > SimplePipelineHandler [2]. However, there are also some pipeline handlers\n> > > > that set them in `CameraConfiguration::validate()` [3] [4].\n> > >\n> > > Indeed most of our documentation suggests that setStream() is meant to\n> > > be called during configure(). Considering that validate() is -always-\n> > > called before configure() by Camera, so where exactly you call it, I\n> > > don't think makes a big difference.\n> > >\n> > > We could also relax the documentation.\n> > >\n> >\n> > Yes, that's a great idea to avoid confusion.\n> >\n> >\n> > >\n> > > >\n> > > > [1]:\n> > > >\n> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n303\n> > > > [2]:\n> > > >\n> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/simple/simple.cpp#n1270\n> > > > [3]:\n> > > >\n> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n347\n> > > > [4]:\n> > > >\n> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rkisp1/rkisp1.cpp#n520\n> > > >\n> > > > Does\n> > > > > the mtkisp7 pipes have different capabilities when it comes to\n> > > > > supported formats and resolutions ?\n> > > > >\n> > > >\n> > > > No, as you can see in mtkisp7's implementation [5], all of them support\n> > > the\n> > > > same formats and resolutions.\n> > > >\n> > > > [5]:\n> > > >\n> > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n> > > >\n> > > >\n> > > >\n> > > >\n> > > > > >\n> > > > > > Does that make sense :P?\n> > > > >\n> > > > > I guess so, but I wonder if the \"role\" is the main criteria that\n> > > > > should be taken into account when assiging pipes to streams.\n> > > > >\n> > > > How would this work for applications that operates on platforms where\n> > > > > the pipe selection depends on other criteria like the supported image\n> > > > > formats and stream resolutions ? In example, for rkisp1\n> > > > > the \"self\" pipelines can do RGB while the \"main\" can't and that's how\n> > > > > we assign pipes during validate().\n> > > >\n> > > >\n> > > > > To be honest this has not proven to be optimal and I'm not opposed to\n> > > > > add Roles to the pipe selection criteria, but they shouldn't be made\n> > > the\n> > > > > only criteria on which pipes are assigned (unless we support this on\n> > > > > all pipelines).\n> > > > >\n> > > > >\n> > > > I agree that the `role` might not be the main criteria in general cases.\n> > > > It's just that we find difficulties in assigning them properly with the\n> > > > current\n> > > > configurations in mtkisp7.\n> > > >\n> > > >\n> > > >\n> > > > > Also I would not based any future-proof design on Roles, they will\n> > > > > likely be heavily reworked or removed going forward.\n> > > > >\n> > > > > As your main use case is Android, I think it would be doable for you\n> > > > > to\n> > > > > 1) Request a generateConfiguration() and keep track of the\n> > > > > StreamConfiguration order.\n> > > > >\n> > > > > generateConfiguration(Viewfinder, Still, Raw)\n> > > > >\n> > > > > will give you StreamConfiguration for the above rols in order as\n> > > > > you have requested them, if supported.\n> > > > >\n> > > > > 2) Code validate() so that you try to assing pipes based on the\n> > > > > formats/sizes and if formats/sizes are the same define an ordering.\n> > > > > In example the first YUV/WxH stream goes to Viewfinder if an\n> > > > > identical YUV/WxH stream is requested for Still Capture.\n> > > > >\n> > > >\n> > > > This actually assumes the application doesn't change the content of the\n> > > > default CameraConfiguration/StreamConfiguration, which I doubt if it's a\n> > > > good thing. Currently in Android adapter, it re-arranges [6]\n> > > > StreamConfigurations based on the previously-fetched default\n> > > > StreamConfigurations of the StreamRoles, which I think is a normal\n> > > > practice of the current interfaces' logic. Please correct me if I'm\n> > > wrong :)\n> > >\n> > > Good point, as Android HAL re-sorts the StreamConfiguration before\n> > > calling Camera::configure() you can't rely on the order in which you\n> > > have requested roles to Camera::generateConfiguration().\n> > >\n> > > >\n> > > > [6]:\n> > > >\n> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n708\n> > > >\n> > > > Also, if an application calls\n> > > > `generateConfiguration(camera, {StreamRole::VideoRecording})` and\n> > > > `generateConfiguration(camera, {StreamRole::StillCapture})`\n> > > > respectively, and ends up calling `configure()` with `VideoRecording`'s\n> > > > CameraConfiguration, how would the pipeline handler know that the\n> > > > application intends to get buffers from the stream `VideoRecording`?\n> > > > In your suggestion, it seems that the application needs to call\n> > > > `generateConfiguration(camera, {StreamRole::VideoRecording})` again,\n> > > > which I don't think is enforced in the current libcamera API, and might\n> > > > not be a good practice.\n> > > >\n> > > >\n> > > > > 3) As validate() assigns Steams * to StreamConfiguration (with\n> > > > > ::setStream) it should be easy to keep track to which pipe a\n> > > > > StreamConfiguration has been assigned to at configure() time.\n> > > > >\n> > > > > Could you list what are the platform's pipes capabilities ?\n> > > > >\n> > > > >\n> > > > For example, on `ciri` with mtkisp7 pipeline handler [5], `pixelFormat`\n> > > is\n> > > > NV12,\n> > > > supported resolution range is `320x240` to `2592x1944`, and\n> > > `bufferCount` is\n> > > > 8. It applies to `StillCapture`, `VideoRecording`, and `ViewFinder`.\n> > > >\n> > > > [5]:\n> > > >\n> > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n> > > >\n> > >\n> > > Looking at the pipeline handler implementation from your tree it seems\n> > > to me that you're using roles in validate() to call setStream(), as we\n> > > already clarified:\n> > >\n> > > switch (cfg.role) {\n> > > case StreamRole::Viewfinder:\n> > > case StreamRole::VideoRecording:\n> > > if (videoCnt >= 2) {\n> > > LOG(MtkISP7, Error)\n> > > << \"Support only 2 Preview/Video\n> > > streams\";\n> > > return Invalid;\n> > > }\n> > > cfg.setStream(const_cast > > *>(vidStreams[videoCnt++]));\n> > > break;\n> > > case StreamRole::StillCapture:\n> > > if (stillCnt >= 2) {\n> > > LOG(MtkISP7, Error)\n> > > << \"Support only 2 StillCapture\n> > > streams\";\n> > > return Invalid;\n> > > }\n> > > cfg.setStream(const_cast > > *>(stillStreams[stillCnt++]));\n> > > break;\n> > > default:\n> > > LOG(MtkISP7, Error) << \"Invalid StreamRole \" <<\n> > > cfg.role;\n> > > return Invalid;\n> > > }\n> > >\n> > > This shows that you have 4 streams, 2 for Preview/Video and 2 for\n> > > StillCapture.\n> > >\n> > > Then you use the Stream in configure() for populate two sizes\n> > >\n> > > Size video1 = Size{ 0, 0 };\n> > > Size video2 = Size{ 0, 0 };\n> > > Size still1 = Size{ 0, 0 };\n> > > Size still2 = Size{ 0, 0 };\n> > >\n> > > /* Only cover the video resolution */\n> > > for (auto &cfg : *c) {\n> > > if (cfg.stream() == &video1Stream_)\n> > > video1 = cfg.size;\n> > > else if (cfg.stream() == &video2Stream_)\n> > > video2 = cfg.size;\n> > > else if (cfg.stream() == &still1Stream_)\n> > > still1 = cfg.size;\n> > > else if (cfg.stream() == &still2Stream_)\n> > > still2 = cfg.size;\n> > > else\n> > > return -EINVAL;\n> > > }\n> > >\n> > > From there on, all the API you have implemented relies on the presence\n> > > and order of the 'video1', 'video2, 'still1' and 'still2' parameters.\n> > >\n> > > In example\n> > >\n> > > mcnrManager.configure(camsysYuvSize, video1, video2);\n> > > lpnrManager.configure(sensorFullSize_, still1, still2);\n> > > lpnrTunManager.configure(sensorFullSize_, still1, still2);\n> > > mcnrTunManager.configure(camsysYuvSize, video1, video2);\n> > >\n> > > Now, according to what I've read and what you said, there are no\n> > > constraints on the HW that help identify Stream. To make an example,\n> > > you can't assume \"Oh this StreamConfiguration is RGB so it needs to go\n> > > to StreamX\".\n> > >\n> >\n> > Yes, that's the main concern :)\n> >\n> >\n> > >\n> > > So I guess what you want is to allow an application to say \"One\n> > > viewfinder is WxH, one Still capture is WxH, one viewfinder is WxH\"\n> > > (also, all Streams are NV12 if I'm not mistaken).\n> > >\n> > > > As said, I'm not opposed to use Roles for assigning pipes, but I don't\n> > > > > think we should based any new development on Roles as there's an high\n> > > > > chance they can be reworked.\n> > > > >\n> > > >\n> > > > Could you briefly describe how the new APIs would be like?\n> > > >\n> > > > Basically I think either applications need to have an argument to\n> > > indicate\n> > > > which kind of stream the StreamConfiguration needs to be assigned to, or\n> > > > the pipeline handler needs to remember which StreamRole a\n> > > > StreamConfiguration returned was asked as the default configuration for.\n> > >\n> > > It's not really about that, the idea is that the stream's\n> > > characteristics should be used to assign a stream to a pipe, like the\n> > > format and the sizes. In your case that's not really possible as all\n> > > streams are NV12 and all resolutions can go anywhere.\n> > >\n> > > But, as you use roles, that logic should live somewhere, doesn't it ?\n> > >\n> > > Looking at your implementation of the Android HAL I see (sorry, cannot\n> > > point out a commit id as there's quite some reverts in the history)\n> > >\n> > > if (isJpegStream(stream)) {\n> > > continue;\n> > > } else if (isYuvSnapshotStream(stream)) {\n> > > streamConfig.streams = { { stream,\n> > > CameraStream::Type::Direct } };\n> > > streamConfig.config.role =\n> > > StreamRole::StillCapture;\n> > > } else if (isPreviewStream(stream)) {\n> > > streamConfig.streams = { { stream,\n> > > CameraStream::Type::Direct } };\n> > > streamConfig.config.role = StreamRole::Viewfinder;\n> > > } else if (isVideoStream(stream)) {\n> > > streamConfig.streams = { { stream,\n> > > CameraStream::Type::Direct } };\n> > > streamConfig.config.role =\n> > > StreamRole::VideoRecording;\n> > > } else {\n> > > streamConfig.streams = { { stream,\n> > > CameraStream::Type::Direct } };\n> > > streamConfig.config.role = StreamRole::Viewfinder;\n> > > }\n> > >\n> > > So you populate roles based on some criteria here, and if I look at\n> > > the criteria\n> > >\n> > > bool isPreviewStream(camera3_stream_t *stream)\n> > > {\n> > > return (GRALLOC_USAGE_HW_COMPOSER & stream->usage);\n> > > }\n> > >\n> > > bool isVideoStream(camera3_stream_t *stream)\n> > > {\n> > > return (GRALLOC_USAGE_HW_VIDEO_ENCODER & stream->usage);\n> > > }\n> > >\n> > > bool isYuvSnapshotStream(camera3_stream_t *stream)\n> > > {\n> > > return (!isVideoStream(stream) && !isPreviewStream(stream) &&\n> > > (HAL_PIXEL_FORMAT_YCbCr_420_888 == stream->format));\n> > > }\n> > >\n> > > they indeed come from Android's requirements that cannot be expressed\n> > > in libcamera.\n> > >\n> > > To be clear, when asking \"what you need this for\" this is the level of\n> > > detail that is needed to explain your design choices. Otherwise it's\n> > > up to us to decode what you have done in the mtk support branch.\n> > >\n> >\n> > I think that's also related to another confusion that I have regarding the\n> > current libcamera API: We use `StreamRole` as the input to get the\n> > default configuration, while it doesn't stop a pipeline handler to assign\n> > it to a different stream, with a different StreamRole, later in `validate()`\n>\n> Which makes me wonder, if we allow apps to set StreamConfig::role, how\n> are we going to validate it ?\n\nI assume you mean that if a StreamConfiguration contains StreamRole\nalong with other fields, and they conflict with each other with apps'\nmanipulation, how can the pipeline handler validate it?\nIn this case, I think the pipeline handler should return Invalid directly,\nor fix the StreamConfiguration properly and return Adjusted.\n\n>\n> > or `configure()`. The pipeline handler might not even return\n> > `Status::Adjusted`, if the requested arguments are not changed.\n>\n> The thing is that StreamRoles usage should have been limited to\n> generateConfiguration() only. It shouldn't be related to which Stream\n> in the pipeline handler a StreamConfig is assigned to, as there's no\n> 1-to-1 matching between StreamRoles (libcamera API) and the number and\n> charateristics of a Stream (platform specific).\n\nI agree that the 1-to-1 matching doesn't exist. Take mtkisp7 as the\nexample, video1Stream_ & video2Stream_ can support both\nStreamRole::VideoRecording & StreamRole::Viewfinder, and\nstill1Stream & still2Stream support only StreamRole::StillCapture.\n\nHowever, what I mean here is that when an app validates/configures\na StreamConfiguration, which was updated from StreamRole::A, it\nshould be assigned to a stream that supports StreamRole::A.\n(Unless the app updates its fields to be totally different.)\n\nBR,\nHarvey\n\n>\n> >\n> > Maybe libcamera core libraries assume each ISP pipe has different\n> > characteristics, like PixelFormat?\n> >\n>\n> I presume so.\n>\n> >\n> > >\n> > >\n> > > > It doesn't have to be `StreamRole`. If there are other types/arguments in\n> > > > the new reworked API, I'd be happy to adapt to the new one(s).\n> > > >\n> > > > Two naive ideas:\n> > > > 1. Keep the new `StreamRole role;` to be a const member variable in\n> > > > StreamConfiguration, which should be set in\n> > > > `PipelineHandler::generateConfiguration()`.\n> > > >\n> > > > 2. If it makes sense, add a private class for `StreamConfiguration` and\n> > > keep\n> > > > the new `StreamRole role;` there, so that applications cannot get the\n> > > info.\n> > > > (I don't think we need to hide the info from the applications though...)\n> > > >\n> > > > WDYT?\n> > >\n> > > Given the above described use case is my opinion valid, I wouldn't be\n> > > opposed to have roles as a public member of the StreamConfiguration to\n> > > allow application to hint how a stream should be used if no other\n> > > criteria is applicable.\n> > >\n> >\n> > Yeah I also think that the current patch is the simplest.\n> >\n> >\n> > >\n> > > >\n> > > >\n> > > > > Cc-Laurent for opinions.\n> > >\n> > > As this is an application-facing API change, I would however like to\n> > > see more opinions.\n> > >\n> >\n> > Sure, let's wait for more opinions from others :)\n> >\n>\n> I'll make sure to discuss it with them in the next days\n>\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > > >\n> > > > > >\n> > > > > > BR,\n> > > > > > Harvey\n> > > > >\n> > > >\n> > > >\n> > > > --\n> > > > BR,\n> > > > Harvey Yang\n> > >\n> >\n> >\n> > --\n> > BR,\n> > Harvey Yang","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 7F831C3257\n\tfor ;\n\tTue, 24 Sep 2024 06:42:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 75E2C6350B;\n\tTue, 24 Sep 2024 08:42:57 +0200 (CEST)","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A15CD618DA\n\tfor ;\n\tTue, 24 Sep 2024 08:42:55 +0200 (CEST)","by mail-lj1-x235.google.com with SMTP id\n\t38308e7fff4ca-2f759b87f83so53910231fa.2\n\tfor ;\n\tMon, 23 Sep 2024 23:42:55 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"QqL1Qw1x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1727160175; x=1727764975;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=94DQZhfb2gVs9yXIf7vJIcd9dYeCimz61MST5UtD1H4=;\n\tb=QqL1Qw1xx3dWKiPvnT57esmes96a7QAFD+W1loOVhwXINgSxEkFtgtOWNw0vZysiQx\n\tyfKD/veHQig37QFGepompFfWah5yD03DDZQDa5YZSq76t6nAlcOZirOi2AWFiaoi5LGp\n\tlQF31Nu1eyUBz4bszVJtX3dpIvh/EBZNwU8pU=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1727160175; x=1727764975;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=94DQZhfb2gVs9yXIf7vJIcd9dYeCimz61MST5UtD1H4=;\n\tb=dCwqjkQUcSKU7wcvjZOBlFH39jg94gBne5PW1+UvyTIsEPDjo8xz5QT0LrVHW5U/lZ\n\twtXo++a9iDXEHwLiOPxzeZDmqbWq1LFtBrhEa+hTlC2W/sBHD8qS0RRpoa0Fx9Pa7ZxH\n\t+VfuXV/i0h7aLyhEqpLgn6Awos7qp1xAqBR9/2BWa4sHHj72YgY4Oql7fWAkNQczI6CM\n\tUoSzArehPNx63jai2QtyKNhO6uMorGppJ1Uyw8pKpAUCdK11rUdrimV1TonvAeqiLyYy\n\tZuow94pA78r01lsPxVLTqy1qPPY/f4yt52da06frppEAN/ZB04F85SHJ+uexlYiGQiML\n\tMDTQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXWQs85QduPWACrFcj9dQdajmGTEtwsqKfAwHjaJUkuxWnNq1bNhiKJDzWbPsPfu9OZV6miqUn9tTvKOOHXKeQ=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yw0/omPgcaVVqJgxcuBYnw/PX4ALo/MlqBUFCzTX5oKMmvvZyej\n\t7utQeEe4VpM4Nhq08AiqCyLgY2jIyq+jWQaaoH9QFoqErjyhACRt8QM4jM0xx68P3Z6w8cl0Tsz\n\trjDt0ipAopu0vfwImrOyZ2Gj4bwq7LZJLrkT6DRdGqCsfHlg=","X-Google-Smtp-Source":"AGHT+IGGbVnNUNxKh2/BadmPDOX4K+6JA3DC0sVBtmg+ya+BnGVNu0jzSYgNKvKCmyk5hDx1KB9FIzhe/xYg76ksZBc=","X-Received":"by 2002:a2e:be8f:0:b0:2f6:5f7b:e5cf with SMTP id\n\t38308e7fff4ca-2f7cc365cfbmr67489341fa.14.1727160174600;\n\tMon, 23 Sep 2024 23:42:54 -0700 (PDT)","MIME-Version":"1.0","References":"<20240916045802.3799103-1-chenghaoyang@google.com>\n\t\n\t\n\t\n\t\n\t\n\t\n\t<5imcxykorjceprfzhynsijjian7p55fn5vry6ogfajsanczian@xtg6ooszbxo3>\n\t\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Tue, 24 Sep 2024 14:42:43 +0800","Message-ID":"","Subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","To":"Jacopo Mondi ","Cc":"Cheng-Hao Yang , \n\tLaurent Pinchart , \n\tkieran bingham ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31336,"web_url":"https://patchwork.libcamera.org/comment/31336/","msgid":"<217c005cf90c9d877fc4a181a45f750950ac3e23.camel@ndufresne.ca>","date":"2024-09-24T12:52:04","subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mardi 24 septembre 2024 à 13:29 +0800, Cheng-Hao Yang a écrit :\n> On Tue, Sep 24, 2024 at 2:36 AM Nicolas Dufresne wrote:\n> > \n> > \n> > Hi,\n> > \n> > Le lundi 23 septembre 2024 à 22:14 +0800, Cheng-Hao Yang a écrit :\n> > > Thanks Jacopo for looking into the tree of the mtkisp7 branch!\n> > \n> > Small technical note, it will be appreciated by many users if you reply in text\n> > form. This is true for most mailing list out there.\n> \n> Ah okay. Hope I'm doing it right this way.\n\nThank you, this is exactly what I meant.\n\nNicolas\n\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 A13DAC0F1B\n\tfor ;\n\tTue, 24 Sep 2024 12:52:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A55E263510;\n\tTue, 24 Sep 2024 14:52:08 +0200 (CEST)","from mail-vs1-xe30.google.com (mail-vs1-xe30.google.com\n\t[IPv6:2607:f8b0:4864:20::e30])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 241C163500\n\tfor ;\n\tTue, 24 Sep 2024 14:52:07 +0200 (CEST)","by mail-vs1-xe30.google.com with SMTP id\n\tada2fe7eead31-49bd32f6a11so1883317137.1\n\tfor ;\n\tTue, 24 Sep 2024 05:52:07 -0700 (PDT)","from nicolas-tpx395.lan ([2606:6d00:15:862e::580])\n\tby smtp.gmail.com with ESMTPSA id\n\t6a1803df08f44-6cb0f4c6082sm6285386d6.55.2024.09.24.05.52.05\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 24 Sep 2024 05:52:05 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ndufresne-ca.20230601.gappssmtp.com\n\theader.i=@ndufresne-ca.20230601.gappssmtp.com\n\theader.b=\"zrltWqRm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20230601.gappssmtp.com; s=20230601; t=1727182326;\n\tx=1727787126; darn=lists.libcamera.org; \n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:from:to:cc:subject\n\t:date:message-id:reply-to;\n\tbh=fTQxs8jLVAI89pd6W8Ig0n3K9WjQS2wKEqi47OXltXU=;\n\tb=zrltWqRmCjB60xy1LFc/eXA5N+UR/w3m5jXKCrFCSfl7++DsqKv6Zo27444vUN7XI4\n\tV97H6oHIVECUPyOvkPYxQ86PEWcJAE8OMsCq/KFXLga8EBgxQVxlroijfgGjv7pWcPDP\n\tUTKN8rEJDzX/mvZBGD+oPl9NjIeGpNE6C8WsP1N8lQ4l/zO463QtWkxaQLsogPZeIyZq\n\tCipY44bw30LedA5zbjpcVybtxPFnShea6z0AVroUf2T6dNVvIJznGcxq2r3oKghgjrmo\n\te8/+fVWhniuVS8uMX5q3I4+OsDb3cn8FssdtYKuKaPNFEob/0DHWccq59NSQmxH/LlHo\n\tswaQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1727182326; x=1727787126;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=fTQxs8jLVAI89pd6W8Ig0n3K9WjQS2wKEqi47OXltXU=;\n\tb=XNoZvHonKv7x8fNBo9x6MasXrzCbCS/8Bpv8qeR1s/gdb9o0vQRNdakKpT7IK5Xem8\n\tZXyDSqUWYMMdbfIF9PQ8mnCaBHASdFdWFt+lOpI/R3C1Q8cxf3Jb6lwlZtmph6L+sYJ5\n\tIb7Iu1nbq+0Bea9VHtzRFQcYWJW5IZX7fKbdyMEuKU2sKIbYscy0Q8j5qfH6WmuLdXOc\n\tQSEg4b+EW/plUtxSJjifjoGlkh+s/OLWXwIDqimr7f37oko+pGm0New5yWMEZjbqF66P\n\td42AFglQ3MCiyE3uZMKFsObmYUZtLpLz9/isr95P9EANflJZiJc4RloSPhh+Wawvx7S/\n\tSDKg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVXs08GmWSmQ9xvvNlxZj3HiDYU1uvicGMLVlorIoMk/g4Mxq/hP3DomuffHJxweT4GjJu30GfhrUzWi0wMsSs=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yx2EedWXhuGSL02FdKHB/vtbEDQfklRoaGA1wqEvR2zS4dNPpi7\n\tQ74s7s7tuEwHKriZHH59U0VRXb5NkLp93KgW9vWwf9Aoxj5wrrUTGaaq5OC/8X/E+ezWsQoer1v\n\t6","X-Google-Smtp-Source":"AGHT+IHNDq8W+/CnSFJOFUcSEnv2+0BQRyLiCkNYwMbJy2vWaRT0EJ0GeqpquTdga3Eol2f1jb6qKw==","X-Received":"by 2002:a67:fd6a:0:b0:4a1:50fc:ab77 with SMTP id\n\tada2fe7eead31-4a150fcac02mr1310544137.12.1727182325991; \n\tTue, 24 Sep 2024 05:52:05 -0700 (PDT)","Message-ID":"<217c005cf90c9d877fc4a181a45f750950ac3e23.camel@ndufresne.ca>","Subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","From":"Nicolas Dufresne ","To":"Cheng-Hao Yang ","Cc":"Cheng-Hao Yang , Jacopo Mondi\n\t, Laurent Pinchart\n\t, kieran bingham\n\t, libcamera-devel@lists.libcamera.org","Date":"Tue, 24 Sep 2024 08:52:04 -0400","In-Reply-To":"","References":"<20240916045802.3799103-1-chenghaoyang@google.com>\n\t\n\t\n\t\n\t\n\t\n\t\n\t<5imcxykorjceprfzhynsijjian7p55fn5vry6ogfajsanczian@xtg6ooszbxo3>\n\t\n\t<875f9b54c80432b4d530ae821a7ad6db75b4499e.camel@ndufresne.ca>\n\t","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.52.4 (3.52.4-1.fc40) ","MIME-Version":"1.0","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31426,"web_url":"https://patchwork.libcamera.org/comment/31426/","msgid":"<20240926231346.GM21788@pendragon.ideasonboard.com>","date":"2024-09-26T23:13:46","subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Harvey,\n\nI finally managed to go through the conversation. I'll top-post as\nthere's one question I haven't seen being discussed.\n\nThe mail thread has focussed on the mtkisp7, whose outputs all support\nthe same formats and are therefore not distinguishable from each other\nwith the current API. You've proposed using stream roles to\ndifferentiate between the outputs. Before discussing what the right\nsolution is, I'd like to understand how the outputs differ from each\nother. Surely if they all had the exact same capabilities we wouldn't\nhave this conversation, so how do they differ ? I think understanding\nthis will be key to designing the right API.\n\nOn Tue, Sep 24, 2024 at 02:42:43PM +0800, Cheng-Hao Yang wrote:\n> On Mon, Sep 23, 2024 at 11:10 PM Jacopo Mondi wrote:\n> > On Mon, Sep 23, 2024 at 10:14:48PM GMT, Cheng-Hao Yang wrote:\n> > > On Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi wrote:\n> > > > On Thu, Sep 19, 2024 at 09:55:58PM GMT, Cheng-Hao Yang wrote:\n> > > > > On Thu, Sep 19, 2024 at 5:09 PM Jacopo Mondi wrote:\n> > > > > > On Wed, Sep 18, 2024 at 04:33:27PM GMT, Cheng-Hao Yang wrote:\n> > > > > > > On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi wrote:\n> > > > > > > > On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:\n> > > > > > > > > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi wrote:\n> > > > > > > > > > On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:\n> > > > > > > > > > > Hi folks,\n> > > > > > > > > > >\n> > > > > > > > > > > Currently applications set resolutions, pixelFormat, bufferCount, etc,\n> > > > > > > > > > > into StreamConfigurations, and Pipeline Handler decides which streams\n> > > > > > > > > > > they're assigned to. However, it doesn't allow application to assign\n> > > > > > > > > > > streams that cannot be distinguished by those arguments into\n> > > > > > > > > > > VideoRecording or StillCapture (say YUV/NV12 format), which is needed in\n> > > > > > > > > > > mtkisp7.\n> > > > > > > > > >\n> > > > > > > > > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > > > > > > > > you plan to use StreamRole as part of the stream configuration ?\n> > > > > > > >\n> > > > > > > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > > > > > > you plan to use StreamRole as part of the stream configuration ?\n> > > > > > >\n> > > > > > > In mtkisp7 pipeline handler, both preview (or video) and still capture\n> > > > > > > streams support the same format (NV12) and bufferCount, and the\n> > > > > > > maximum resolutions are also the same. Therefore, when calling\n> > > > > > > `validate()` and `configure()`, mtkisp7 pipeline handler doesn't know\n> > > > > > > if applications/Android adapter wants the stream to go through the\n> > > > > > > still captur pipeline or not.\n> > > > > >\n> > > > > > I still have an hard time understanding why validate() and configure()\n> > > > > > needs to know the \"role\". Is this to assign \"pipes\" to Streams ?\n> > > > >\n> > > > > If I understand correctly that \"pipes\" means the pipelines of ISP/IPA algos\n> > > > > for preview/video/still capture respectively, yes, it's to assign the\n> > > > > StreamConfiguration(s) to the desired pipe(s), which means to call\n> > > > > `StreamConfiguration::setStream()`. According to the comments [1], it's\n> > > > > supposed to be called in `PipelineHandler::configure()`, like\n> > > > > SimplePipelineHandler [2]. However, there are also some pipeline handlers\n> > > > > that set them in `CameraConfiguration::validate()` [3] [4].\n> > > >\n> > > > Indeed most of our documentation suggests that setStream() is meant to\n> > > > be called during configure(). Considering that validate() is -always-\n> > > > called before configure() by Camera, so where exactly you call it, I\n> > > > don't think makes a big difference.\n> > > >\n> > > > We could also relax the documentation.\n> > >\n> > > Yes, that's a great idea to avoid confusion.\n> > >\n> > > > > [1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n303\n> > > > > [2]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/simple/simple.cpp#n1270\n> > > > > [3]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n347\n> > > > > [4]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rkisp1/rkisp1.cpp#n520\n> > > > >\n> > > > > > Does\n> > > > > > the mtkisp7 pipes have different capabilities when it comes to\n> > > > > > supported formats and resolutions ?\n> > > > >\n> > > > > No, as you can see in mtkisp7's implementation [5], all of them support the\n> > > > > same formats and resolutions.\n> > > > >\n> > > > > [5]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n> > > > >\n> > > > > > > Does that make sense :P?\n> > > > > >\n> > > > > > I guess so, but I wonder if the \"role\" is the main criteria that\n> > > > > > should be taken into account when assiging pipes to streams.\n> > > > > >\n> > > > > > How would this work for applications that operates on platforms where\n> > > > > > the pipe selection depends on other criteria like the supported image\n> > > > > > formats and stream resolutions ? In example, for rkisp1\n> > > > > > the \"self\" pipelines can do RGB while the \"main\" can't and that's how\n> > > > > > we assign pipes during validate().\n> > > > >\n> > > > >\n> > > > > > To be honest this has not proven to be optimal and I'm not opposed to\n> > > > > > add Roles to the pipe selection criteria, but they shouldn't be made the\n> > > > > > only criteria on which pipes are assigned (unless we support this on\n> > > > > > all pipelines).\n> > > > >\n> > > > > I agree that the `role` might not be the main criteria in general cases.\n> > > > > It's just that we find difficulties in assigning them properly with the\n> > > > > current\n> > > > > configurations in mtkisp7.\n> > > > >\n> > > > > > Also I would not based any future-proof design on Roles, they will\n> > > > > > likely be heavily reworked or removed going forward.\n> > > > > >\n> > > > > > As your main use case is Android, I think it would be doable for you\n> > > > > > to\n> > > > > > 1) Request a generateConfiguration() and keep track of the\n> > > > > > StreamConfiguration order.\n> > > > > >\n> > > > > > generateConfiguration(Viewfinder, Still, Raw)\n> > > > > >\n> > > > > > will give you StreamConfiguration for the above rols in order as\n> > > > > > you have requested them, if supported.\n> > > > > >\n> > > > > > 2) Code validate() so that you try to assing pipes based on the\n> > > > > > formats/sizes and if formats/sizes are the same define an ordering.\n> > > > > > In example the first YUV/WxH stream goes to Viewfinder if an\n> > > > > > identical YUV/WxH stream is requested for Still Capture.\n> > > > > >\n> > > > >\n> > > > > This actually assumes the application doesn't change the content of the\n> > > > > default CameraConfiguration/StreamConfiguration, which I doubt if it's a\n> > > > > good thing. Currently in Android adapter, it re-arranges [6]\n> > > > > StreamConfigurations based on the previously-fetched default\n> > > > > StreamConfigurations of the StreamRoles, which I think is a normal\n> > > > > practice of the current interfaces' logic. Please correct me if I'm wrong :)\n> > > >\n> > > > Good point, as Android HAL re-sorts the StreamConfiguration before\n> > > > calling Camera::configure() you can't rely on the order in which you\n> > > > have requested roles to Camera::generateConfiguration().\n> > > >\n> > > > > [6]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n708\n> > > > >\n> > > > > Also, if an application calls\n> > > > > `generateConfiguration(camera, {StreamRole::VideoRecording})` and\n> > > > > `generateConfiguration(camera, {StreamRole::StillCapture})`\n> > > > > respectively, and ends up calling `configure()` with `VideoRecording`'s\n> > > > > CameraConfiguration, how would the pipeline handler know that the\n> > > > > application intends to get buffers from the stream `VideoRecording`?\n> > > > > In your suggestion, it seems that the application needs to call\n> > > > > `generateConfiguration(camera, {StreamRole::VideoRecording})` again,\n> > > > > which I don't think is enforced in the current libcamera API, and might\n> > > > > not be a good practice.\n> > > > >\n> > > > >\n> > > > > > 3) As validate() assigns Steams * to StreamConfiguration (with\n> > > > > > ::setStream) it should be easy to keep track to which pipe a\n> > > > > > StreamConfiguration has been assigned to at configure() time.\n> > > > > >\n> > > > > > Could you list what are the platform's pipes capabilities ?\n> > > > > >\n> > > > > >\n> > > > > For example, on `ciri` with mtkisp7 pipeline handler [5], `pixelFormat` is\n> > > > > NV12,\n> > > > > supported resolution range is `320x240` to `2592x1944`, and `bufferCount` is\n> > > > > 8. It applies to `StillCapture`, `VideoRecording`, and `ViewFinder`.\n> > > > >\n> > > > > [5]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n> > > > >\n> > > >\n> > > > Looking at the pipeline handler implementation from your tree it seems\n> > > > to me that you're using roles in validate() to call setStream(), as we\n> > > > already clarified:\n> > > >\n> > > > switch (cfg.role) {\n> > > > case StreamRole::Viewfinder:\n> > > > case StreamRole::VideoRecording:\n> > > > if (videoCnt >= 2) {\n> > > > LOG(MtkISP7, Error)\n> > > > << \"Support only 2 Preview/Video streams\";\n> > > > return Invalid;\n> > > > }\n> > > > cfg.setStream(const_cast(vidStreams[videoCnt++]));\n> > > > break;\n> > > > case StreamRole::StillCapture:\n> > > > if (stillCnt >= 2) {\n> > > > LOG(MtkISP7, Error)\n> > > > << \"Support only 2 StillCapture streams\";\n> > > > return Invalid;\n> > > > }\n> > > > cfg.setStream(const_cast(stillStreams[stillCnt++]));\n> > > > break;\n> > > > default:\n> > > > LOG(MtkISP7, Error) << \"Invalid StreamRole \" << cfg.role;\n> > > > return Invalid;\n> > > > }\n> > > >\n> > > > This shows that you have 4 streams, 2 for Preview/Video and 2 for\n> > > > StillCapture.\n> > > >\n> > > > Then you use the Stream in configure() for populate two sizes\n> > > >\n> > > > Size video1 = Size{ 0, 0 };\n> > > > Size video2 = Size{ 0, 0 };\n> > > > Size still1 = Size{ 0, 0 };\n> > > > Size still2 = Size{ 0, 0 };\n> > > >\n> > > > /* Only cover the video resolution */\n> > > > for (auto &cfg : *c) {\n> > > > if (cfg.stream() == &video1Stream_)\n> > > > video1 = cfg.size;\n> > > > else if (cfg.stream() == &video2Stream_)\n> > > > video2 = cfg.size;\n> > > > else if (cfg.stream() == &still1Stream_)\n> > > > still1 = cfg.size;\n> > > > else if (cfg.stream() == &still2Stream_)\n> > > > still2 = cfg.size;\n> > > > else\n> > > > return -EINVAL;\n> > > > }\n> > > >\n> > > > From there on, all the API you have implemented relies on the presence\n> > > > and order of the 'video1', 'video2, 'still1' and 'still2' parameters.\n> > > >\n> > > > In example\n> > > >\n> > > > mcnrManager.configure(camsysYuvSize, video1, video2);\n> > > > lpnrManager.configure(sensorFullSize_, still1, still2);\n> > > > lpnrTunManager.configure(sensorFullSize_, still1, still2);\n> > > > mcnrTunManager.configure(camsysYuvSize, video1, video2);\n> > > >\n> > > > Now, according to what I've read and what you said, there are no\n> > > > constraints on the HW that help identify Stream. To make an example,\n> > > > you can't assume \"Oh this StreamConfiguration is RGB so it needs to go\n> > > > to StreamX\".\n> > >\n> > > Yes, that's the main concern :)\n> > >\n> > > > So I guess what you want is to allow an application to say \"One\n> > > > viewfinder is WxH, one Still capture is WxH, one viewfinder is WxH\"\n> > > > (also, all Streams are NV12 if I'm not mistaken).\n> > > >\n> > > > > > As said, I'm not opposed to use Roles for assigning pipes, but I don't\n> > > > > > think we should based any new development on Roles as there's an high\n> > > > > > chance they can be reworked.\n> > > > >\n> > > > > Could you briefly describe how the new APIs would be like?\n> > > > >\n> > > > > Basically I think either applications need to have an argument to indicate\n> > > > > which kind of stream the StreamConfiguration needs to be assigned to, or\n> > > > > the pipeline handler needs to remember which StreamRole a\n> > > > > StreamConfiguration returned was asked as the default configuration for.\n> > > >\n> > > > It's not really about that, the idea is that the stream's\n> > > > characteristics should be used to assign a stream to a pipe, like the\n> > > > format and the sizes. In your case that's not really possible as all\n> > > > streams are NV12 and all resolutions can go anywhere.\n> > > >\n> > > > But, as you use roles, that logic should live somewhere, doesn't it ?\n> > > >\n> > > > Looking at your implementation of the Android HAL I see (sorry, cannot\n> > > > point out a commit id as there's quite some reverts in the history)\n> > > >\n> > > > if (isJpegStream(stream)) {\n> > > > continue;\n> > > > } else if (isYuvSnapshotStream(stream)) {\n> > > > streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> > > > streamConfig.config.role = StreamRole::StillCapture;\n> > > > } else if (isPreviewStream(stream)) {\n> > > > streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> > > > streamConfig.config.role = StreamRole::Viewfinder;\n> > > > } else if (isVideoStream(stream)) {\n> > > > streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> > > > streamConfig.config.role = StreamRole::VideoRecording;\n> > > > } else {\n> > > > streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> > > > streamConfig.config.role = StreamRole::Viewfinder;\n> > > > }\n> > > >\n> > > > So you populate roles based on some criteria here, and if I look at\n> > > > the criteria\n> > > >\n> > > > bool isPreviewStream(camera3_stream_t *stream)\n> > > > {\n> > > > return (GRALLOC_USAGE_HW_COMPOSER & stream->usage);\n> > > > }\n> > > >\n> > > > bool isVideoStream(camera3_stream_t *stream)\n> > > > {\n> > > > return (GRALLOC_USAGE_HW_VIDEO_ENCODER & stream->usage);\n> > > > }\n> > > >\n> > > > bool isYuvSnapshotStream(camera3_stream_t *stream)\n> > > > {\n> > > > return (!isVideoStream(stream) && !isPreviewStream(stream) &&\n> > > > (HAL_PIXEL_FORMAT_YCbCr_420_888 == stream->format));\n> > > > }\n> > > >\n> > > > they indeed come from Android's requirements that cannot be expressed\n> > > > in libcamera.\n> > > >\n> > > > To be clear, when asking \"what you need this for\" this is the level of\n> > > > detail that is needed to explain your design choices. Otherwise it's\n> > > > up to us to decode what you have done in the mtk support branch.\n> > > >\n> > >\n> > > I think that's also related to another confusion that I have regarding the\n> > > current libcamera API: We use `StreamRole` as the input to get the\n> > > default configuration, while it doesn't stop a pipeline handler to assign\n> > > it to a different stream, with a different StreamRole, later in `validate()`\n> >\n> > Which makes me wonder, if we allow apps to set StreamConfig::role, how\n> > are we going to validate it ?\n> \n> I assume you mean that if a StreamConfiguration contains StreamRole\n> along with other fields, and they conflict with each other with apps'\n> manipulation, how can the pipeline handler validate it?\n> In this case, I think the pipeline handler should return Invalid directly,\n> or fix the StreamConfiguration properly and return Adjusted.\n> \n> > > or `configure()`. The pipeline handler might not even return\n> > > `Status::Adjusted`, if the requested arguments are not changed.\n> >\n> > The thing is that StreamRoles usage should have been limited to\n> > generateConfiguration() only. It shouldn't be related to which Stream\n> > in the pipeline handler a StreamConfig is assigned to, as there's no\n> > 1-to-1 matching between StreamRoles (libcamera API) and the number and\n> > charateristics of a Stream (platform specific).\n> \n> I agree that the 1-to-1 matching doesn't exist. Take mtkisp7 as the\n> example, video1Stream_ & video2Stream_ can support both\n> StreamRole::VideoRecording & StreamRole::Viewfinder, and\n> still1Stream & still2Stream support only StreamRole::StillCapture.\n> \n> However, what I mean here is that when an app validates/configures\n> a StreamConfiguration, which was updated from StreamRole::A, it\n> should be assigned to a stream that supports StreamRole::A.\n> (Unless the app updates its fields to be totally different.)\n> \n> > > Maybe libcamera core libraries assume each ISP pipe has different\n> > > characteristics, like PixelFormat?\n> >\n> > I presume so.\n> >\n> > > > > It doesn't have to be `StreamRole`. If there are other types/arguments in\n> > > > > the new reworked API, I'd be happy to adapt to the new one(s).\n> > > > >\n> > > > > Two naive ideas:\n> > > > > 1. Keep the new `StreamRole role;` to be a const member variable in\n> > > > > StreamConfiguration, which should be set in\n> > > > > `PipelineHandler::generateConfiguration()`.\n> > > > >\n> > > > > 2. If it makes sense, add a private class for `StreamConfiguration` and keep\n> > > > > the new `StreamRole role;` there, so that applications cannot get the info.\n> > > > > (I don't think we need to hide the info from the applications though...)\n> > > > >\n> > > > > WDYT?\n> > > >\n> > > > Given the above described use case is my opinion valid, I wouldn't be\n> > > > opposed to have roles as a public member of the StreamConfiguration to\n> > > > allow application to hint how a stream should be used if no other\n> > > > criteria is applicable.\n> > >\n> > > Yeah I also think that the current patch is the simplest.\n> > >\n> > > > > > Cc-Laurent for opinions.\n> > > >\n> > > > As this is an application-facing API change, I would however like to\n> > > > see more opinions.\n> > >\n> > > Sure, let's wait for more opinions from others :)\n> >\n> > I'll make sure to discuss it with them in the next days","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 5DD33C0F1B\n\tfor ;\n\tThu, 26 Sep 2024 23:13:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5DAAC6350F;\n\tFri, 27 Sep 2024 01:13:51 +0200 (CEST)","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 CC3C3634F4\n\tfor ;\n\tFri, 27 Sep 2024 01:13:49 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DFA6122F;\n\tFri, 27 Sep 2024 01:12:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XnNRWCh5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727392341;\n\tbh=8ePLZD00Y69LBmbMfZF3pLUv8iYTHTNJGQFq5xS/YcQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XnNRWCh51fXhYuxpZdqMTO72iu7ngrRWNGSVLuDTt9czFM+pXLiOnE6wBOkGWTokz\n\t2pGKNTls4+nOTNON3M8HV2UhZ5CyHLyNfY6dSzOZ7fvxrrvyVHUfyDnTrKZXf6GwDf\n\t13erp9q6Qt6QzO1gYZpoWdRShULiYKweNihGNGgg=","Date":"Fri, 27 Sep 2024 02:13:46 +0300","From":"Laurent Pinchart ","To":"Cheng-Hao Yang ","Cc":"Jacopo Mondi ,\n\tCheng-Hao Yang ,\n\tkieran bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","Message-ID":"<20240926231346.GM21788@pendragon.ideasonboard.com>","References":"\n\t\n\t\n\t\n\t\n\t\n\t<5imcxykorjceprfzhynsijjian7p55fn5vry6ogfajsanczian@xtg6ooszbxo3>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31453,"web_url":"https://patchwork.libcamera.org/comment/31453/","msgid":"","date":"2024-09-27T16:35:22","subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","submitter":{"id":148,"url":"https://patchwork.libcamera.org/api/people/148/","name":"Cheng-Hao Yang","email":"chenghaoyang@google.com"},"content":"Hi Laurent,\n\nOn Fri, Sep 27, 2024 at 7:13 AM Laurent Pinchart\n wrote:\n>\n> Hi Harvey,\n>\n> I finally managed to go through the conversation. I'll top-post as\n> there's one question I haven't seen being discussed.\n>\n> The mail thread has focussed on the mtkisp7, whose outputs all support\n> the same formats and are therefore not distinguishable from each other\n> with the current API. You've proposed using stream roles to\n> differentiate between the outputs. Before discussing what the right\n> solution is, I'd like to understand how the outputs differ from each\n> other. Surely if they all had the exact same capabilities we wouldn't\n> have this conversation, so how do they differ ? I think understanding\n> this will be key to designing the right API.\n\nI agree that understanding how the outputs differ from each other is\none of the keys.\nHowever in mtkisp7, I've confirmed with Han-lin and Yaya from\nMediaTek that users/applications can ask for two streams, one Video\nand one StillCapture, with identical characteristics (in terms of the\ncurrent StreamConfiguration fields [1]), and with only Image Quality\ndifferences, as it goes to different noise reduction pipes in the ISP.\n\nRegarding StreamConfiguration fields, we find `PixelFormat`, `Size`,\n`ColorSpace`, and `bufferCount` to be available for pipeline handlers to\ndistinguish stream configurations, while these three can be identical\nfor Video and StillCapture streams in mtkisp7. `stride` and `frameSize`\nshould be set by pipeline handlers.\n\nTherefore, we couldn't find the fields to help distinguish the stream\nconfigurations for mtkisp7.\n\nIn mtkisp7, the preview/video streams go through Motion-Compensated\nNoise Reduction, and still capture streams go through Low-Pass Noise\nReduction or Multi-Frame Noise Reduction. They also might load\ndifferent tuning parameters, while they should be agnostic to the apps.\n\n[1]: https://git.libcamera.org/libcamera/libcamera.git/tree/include/libcamera/stream.h#n40\n\n\n>\n> On Tue, Sep 24, 2024 at 02:42:43PM +0800, Cheng-Hao Yang wrote:\n> > On Mon, Sep 23, 2024 at 11:10 PM Jacopo Mondi wrote:\n> > > On Mon, Sep 23, 2024 at 10:14:48PM GMT, Cheng-Hao Yang wrote:\n> > > > On Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi wrote:\n> > > > > On Thu, Sep 19, 2024 at 09:55:58PM GMT, Cheng-Hao Yang wrote:\n> > > > > > On Thu, Sep 19, 2024 at 5:09 PM Jacopo Mondi wrote:\n> > > > > > > On Wed, Sep 18, 2024 at 04:33:27PM GMT, Cheng-Hao Yang wrote:\n> > > > > > > > On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi wrote:\n> > > > > > > > > On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:\n> > > > > > > > > > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi wrote:\n> > > > > > > > > > > On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:\n> > > > > > > > > > > > Hi folks,\n> > > > > > > > > > > >\n> > > > > > > > > > > > Currently applications set resolutions, pixelFormat, bufferCount, etc,\n> > > > > > > > > > > > into StreamConfigurations, and Pipeline Handler decides which streams\n> > > > > > > > > > > > they're assigned to. However, it doesn't allow application to assign\n> > > > > > > > > > > > streams that cannot be distinguished by those arguments into\n> > > > > > > > > > > > VideoRecording or StillCapture (say YUV/NV12 format), which is needed in\n> > > > > > > > > > > > mtkisp7.\n> > > > > > > > > > >\n> > > > > > > > > > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > > > > > > > > > you plan to use StreamRole as part of the stream configuration ?\n> > > > > > > > >\n> > > > > > > > > Could you explain in a bit more detail why this \"is needed\" and how\n> > > > > > > > > you plan to use StreamRole as part of the stream configuration ?\n> > > > > > > >\n> > > > > > > > In mtkisp7 pipeline handler, both preview (or video) and still capture\n> > > > > > > > streams support the same format (NV12) and bufferCount, and the\n> > > > > > > > maximum resolutions are also the same. Therefore, when calling\n> > > > > > > > `validate()` and `configure()`, mtkisp7 pipeline handler doesn't know\n> > > > > > > > if applications/Android adapter wants the stream to go through the\n> > > > > > > > still captur pipeline or not.\n> > > > > > >\n> > > > > > > I still have an hard time understanding why validate() and configure()\n> > > > > > > needs to know the \"role\". Is this to assign \"pipes\" to Streams ?\n> > > > > >\n> > > > > > If I understand correctly that \"pipes\" means the pipelines of ISP/IPA algos\n> > > > > > for preview/video/still capture respectively, yes, it's to assign the\n> > > > > > StreamConfiguration(s) to the desired pipe(s), which means to call\n> > > > > > `StreamConfiguration::setStream()`. According to the comments [1], it's\n> > > > > > supposed to be called in `PipelineHandler::configure()`, like\n> > > > > > SimplePipelineHandler [2]. However, there are also some pipeline handlers\n> > > > > > that set them in `CameraConfiguration::validate()` [3] [4].\n> > > > >\n> > > > > Indeed most of our documentation suggests that setStream() is meant to\n> > > > > be called during configure(). Considering that validate() is -always-\n> > > > > called before configure() by Camera, so where exactly you call it, I\n> > > > > don't think makes a big difference.\n> > > > >\n> > > > > We could also relax the documentation.\n> > > >\n> > > > Yes, that's a great idea to avoid confusion.\n> > > >\n> > > > > > [1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n303\n> > > > > > [2]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/simple/simple.cpp#n1270\n> > > > > > [3]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n347\n> > > > > > [4]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rkisp1/rkisp1.cpp#n520\n> > > > > >\n> > > > > > > Does\n> > > > > > > the mtkisp7 pipes have different capabilities when it comes to\n> > > > > > > supported formats and resolutions ?\n> > > > > >\n> > > > > > No, as you can see in mtkisp7's implementation [5], all of them support the\n> > > > > > same formats and resolutions.\n> > > > > >\n> > > > > > [5]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n> > > > > >\n> > > > > > > > Does that make sense :P?\n> > > > > > >\n> > > > > > > I guess so, but I wonder if the \"role\" is the main criteria that\n> > > > > > > should be taken into account when assiging pipes to streams.\n> > > > > > >\n> > > > > > > How would this work for applications that operates on platforms where\n> > > > > > > the pipe selection depends on other criteria like the supported image\n> > > > > > > formats and stream resolutions ? In example, for rkisp1\n> > > > > > > the \"self\" pipelines can do RGB while the \"main\" can't and that's how\n> > > > > > > we assign pipes during validate().\n> > > > > >\n> > > > > >\n> > > > > > > To be honest this has not proven to be optimal and I'm not opposed to\n> > > > > > > add Roles to the pipe selection criteria, but they shouldn't be made the\n> > > > > > > only criteria on which pipes are assigned (unless we support this on\n> > > > > > > all pipelines).\n> > > > > >\n> > > > > > I agree that the `role` might not be the main criteria in general cases.\n> > > > > > It's just that we find difficulties in assigning them properly with the\n> > > > > > current\n> > > > > > configurations in mtkisp7.\n> > > > > >\n> > > > > > > Also I would not based any future-proof design on Roles, they will\n> > > > > > > likely be heavily reworked or removed going forward.\n> > > > > > >\n> > > > > > > As your main use case is Android, I think it would be doable for you\n> > > > > > > to\n> > > > > > > 1) Request a generateConfiguration() and keep track of the\n> > > > > > > StreamConfiguration order.\n> > > > > > >\n> > > > > > > generateConfiguration(Viewfinder, Still, Raw)\n> > > > > > >\n> > > > > > > will give you StreamConfiguration for the above rols in order as\n> > > > > > > you have requested them, if supported.\n> > > > > > >\n> > > > > > > 2) Code validate() so that you try to assing pipes based on the\n> > > > > > > formats/sizes and if formats/sizes are the same define an ordering.\n> > > > > > > In example the first YUV/WxH stream goes to Viewfinder if an\n> > > > > > > identical YUV/WxH stream is requested for Still Capture.\n> > > > > > >\n> > > > > >\n> > > > > > This actually assumes the application doesn't change the content of the\n> > > > > > default CameraConfiguration/StreamConfiguration, which I doubt if it's a\n> > > > > > good thing. Currently in Android adapter, it re-arranges [6]\n> > > > > > StreamConfigurations based on the previously-fetched default\n> > > > > > StreamConfigurations of the StreamRoles, which I think is a normal\n> > > > > > practice of the current interfaces' logic. Please correct me if I'm wrong :)\n\nLet me correct myself: I find that Android adapter sets\nStreamConfigurations from\nscratch [2]. It means that Android adapter cares only about\npixelFormat and size.\nMaybe it's not sufficient, and we can update it to use the default ones fetched\nbefore.\n\nIt doesn't change the story though.\n\n[2]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n638\n\n> > > > >\n> > > > > Good point, as Android HAL re-sorts the StreamConfiguration before\n> > > > > calling Camera::configure() you can't rely on the order in which you\n> > > > > have requested roles to Camera::generateConfiguration().\n> > > > >\n> > > > > > [6]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n708\n> > > > > >\n> > > > > > Also, if an application calls\n> > > > > > `generateConfiguration(camera, {StreamRole::VideoRecording})` and\n> > > > > > `generateConfiguration(camera, {StreamRole::StillCapture})`\n> > > > > > respectively, and ends up calling `configure()` with `VideoRecording`'s\n> > > > > > CameraConfiguration, how would the pipeline handler know that the\n> > > > > > application intends to get buffers from the stream `VideoRecording`?\n> > > > > > In your suggestion, it seems that the application needs to call\n> > > > > > `generateConfiguration(camera, {StreamRole::VideoRecording})` again,\n> > > > > > which I don't think is enforced in the current libcamera API, and might\n> > > > > > not be a good practice.\n> > > > > >\n> > > > > >\n> > > > > > > 3) As validate() assigns Steams * to StreamConfiguration (with\n> > > > > > > ::setStream) it should be easy to keep track to which pipe a\n> > > > > > > StreamConfiguration has been assigned to at configure() time.\n> > > > > > >\n> > > > > > > Could you list what are the platform's pipes capabilities ?\n> > > > > > >\n> > > > > > >\n> > > > > > For example, on `ciri` with mtkisp7 pipeline handler [5], `pixelFormat` is\n> > > > > > NV12,\n> > > > > > supported resolution range is `320x240` to `2592x1944`, and `bufferCount` is\n> > > > > > 8. It applies to `StillCapture`, `VideoRecording`, and `ViewFinder`.\n> > > > > >\n> > > > > > [5]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556\n> > > > > >\n> > > > >\n> > > > > Looking at the pipeline handler implementation from your tree it seems\n> > > > > to me that you're using roles in validate() to call setStream(), as we\n> > > > > already clarified:\n> > > > >\n> > > > > switch (cfg.role) {\n> > > > > case StreamRole::Viewfinder:\n> > > > > case StreamRole::VideoRecording:\n> > > > > if (videoCnt >= 2) {\n> > > > > LOG(MtkISP7, Error)\n> > > > > << \"Support only 2 Preview/Video streams\";\n> > > > > return Invalid;\n> > > > > }\n> > > > > cfg.setStream(const_cast(vidStreams[videoCnt++]));\n> > > > > break;\n> > > > > case StreamRole::StillCapture:\n> > > > > if (stillCnt >= 2) {\n> > > > > LOG(MtkISP7, Error)\n> > > > > << \"Support only 2 StillCapture streams\";\n> > > > > return Invalid;\n> > > > > }\n> > > > > cfg.setStream(const_cast(stillStreams[stillCnt++]));\n> > > > > break;\n> > > > > default:\n> > > > > LOG(MtkISP7, Error) << \"Invalid StreamRole \" << cfg.role;\n> > > > > return Invalid;\n> > > > > }\n> > > > >\n> > > > > This shows that you have 4 streams, 2 for Preview/Video and 2 for\n> > > > > StillCapture.\n> > > > >\n> > > > > Then you use the Stream in configure() for populate two sizes\n> > > > >\n> > > > > Size video1 = Size{ 0, 0 };\n> > > > > Size video2 = Size{ 0, 0 };\n> > > > > Size still1 = Size{ 0, 0 };\n> > > > > Size still2 = Size{ 0, 0 };\n> > > > >\n> > > > > /* Only cover the video resolution */\n> > > > > for (auto &cfg : *c) {\n> > > > > if (cfg.stream() == &video1Stream_)\n> > > > > video1 = cfg.size;\n> > > > > else if (cfg.stream() == &video2Stream_)\n> > > > > video2 = cfg.size;\n> > > > > else if (cfg.stream() == &still1Stream_)\n> > > > > still1 = cfg.size;\n> > > > > else if (cfg.stream() == &still2Stream_)\n> > > > > still2 = cfg.size;\n> > > > > else\n> > > > > return -EINVAL;\n> > > > > }\n> > > > >\n> > > > > From there on, all the API you have implemented relies on the presence\n> > > > > and order of the 'video1', 'video2, 'still1' and 'still2' parameters.\n> > > > >\n> > > > > In example\n> > > > >\n> > > > > mcnrManager.configure(camsysYuvSize, video1, video2);\n> > > > > lpnrManager.configure(sensorFullSize_, still1, still2);\n> > > > > lpnrTunManager.configure(sensorFullSize_, still1, still2);\n> > > > > mcnrTunManager.configure(camsysYuvSize, video1, video2);\n> > > > >\n> > > > > Now, according to what I've read and what you said, there are no\n> > > > > constraints on the HW that help identify Stream. To make an example,\n> > > > > you can't assume \"Oh this StreamConfiguration is RGB so it needs to go\n> > > > > to StreamX\".\n> > > >\n> > > > Yes, that's the main concern :)\n> > > >\n> > > > > So I guess what you want is to allow an application to say \"One\n> > > > > viewfinder is WxH, one Still capture is WxH, one viewfinder is WxH\"\n> > > > > (also, all Streams are NV12 if I'm not mistaken).\n> > > > >\n> > > > > > > As said, I'm not opposed to use Roles for assigning pipes, but I don't\n> > > > > > > think we should based any new development on Roles as there's an high\n> > > > > > > chance they can be reworked.\n> > > > > >\n> > > > > > Could you briefly describe how the new APIs would be like?\n> > > > > >\n> > > > > > Basically I think either applications need to have an argument to indicate\n> > > > > > which kind of stream the StreamConfiguration needs to be assigned to, or\n> > > > > > the pipeline handler needs to remember which StreamRole a\n> > > > > > StreamConfiguration returned was asked as the default configuration for.\n> > > > >\n> > > > > It's not really about that, the idea is that the stream's\n> > > > > characteristics should be used to assign a stream to a pipe, like the\n> > > > > format and the sizes. In your case that's not really possible as all\n> > > > > streams are NV12 and all resolutions can go anywhere.\n> > > > >\n> > > > > But, as you use roles, that logic should live somewhere, doesn't it ?\n> > > > >\n> > > > > Looking at your implementation of the Android HAL I see (sorry, cannot\n> > > > > point out a commit id as there's quite some reverts in the history)\n> > > > >\n> > > > > if (isJpegStream(stream)) {\n> > > > > continue;\n> > > > > } else if (isYuvSnapshotStream(stream)) {\n> > > > > streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> > > > > streamConfig.config.role = StreamRole::StillCapture;\n> > > > > } else if (isPreviewStream(stream)) {\n> > > > > streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> > > > > streamConfig.config.role = StreamRole::Viewfinder;\n> > > > > } else if (isVideoStream(stream)) {\n> > > > > streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> > > > > streamConfig.config.role = StreamRole::VideoRecording;\n> > > > > } else {\n> > > > > streamConfig.streams = { { stream, CameraStream::Type::Direct } };\n> > > > > streamConfig.config.role = StreamRole::Viewfinder;\n> > > > > }\n> > > > >\n> > > > > So you populate roles based on some criteria here, and if I look at\n> > > > > the criteria\n> > > > >\n> > > > > bool isPreviewStream(camera3_stream_t *stream)\n> > > > > {\n> > > > > return (GRALLOC_USAGE_HW_COMPOSER & stream->usage);\n> > > > > }\n> > > > >\n> > > > > bool isVideoStream(camera3_stream_t *stream)\n> > > > > {\n> > > > > return (GRALLOC_USAGE_HW_VIDEO_ENCODER & stream->usage);\n> > > > > }\n> > > > >\n> > > > > bool isYuvSnapshotStream(camera3_stream_t *stream)\n> > > > > {\n> > > > > return (!isVideoStream(stream) && !isPreviewStream(stream) &&\n> > > > > (HAL_PIXEL_FORMAT_YCbCr_420_888 == stream->format));\n> > > > > }\n> > > > >\n> > > > > they indeed come from Android's requirements that cannot be expressed\n> > > > > in libcamera.\n> > > > >\n> > > > > To be clear, when asking \"what you need this for\" this is the level of\n> > > > > detail that is needed to explain your design choices. Otherwise it's\n> > > > > up to us to decode what you have done in the mtk support branch.\n> > > > >\n> > > >\n> > > > I think that's also related to another confusion that I have regarding the\n> > > > current libcamera API: We use `StreamRole` as the input to get the\n> > > > default configuration, while it doesn't stop a pipeline handler to assign\n> > > > it to a different stream, with a different StreamRole, later in `validate()`\n> > >\n> > > Which makes me wonder, if we allow apps to set StreamConfig::role, how\n> > > are we going to validate it ?\n> >\n> > I assume you mean that if a StreamConfiguration contains StreamRole\n> > along with other fields, and they conflict with each other with apps'\n> > manipulation, how can the pipeline handler validate it?\n> > In this case, I think the pipeline handler should return Invalid directly,\n> > or fix the StreamConfiguration properly and return Adjusted.\n> >\n> > > > or `configure()`. The pipeline handler might not even return\n> > > > `Status::Adjusted`, if the requested arguments are not changed.\n> > >\n> > > The thing is that StreamRoles usage should have been limited to\n> > > generateConfiguration() only. It shouldn't be related to which Stream\n> > > in the pipeline handler a StreamConfig is assigned to, as there's no\n> > > 1-to-1 matching between StreamRoles (libcamera API) and the number and\n> > > charateristics of a Stream (platform specific).\n> >\n> > I agree that the 1-to-1 matching doesn't exist. Take mtkisp7 as the\n> > example, video1Stream_ & video2Stream_ can support both\n> > StreamRole::VideoRecording & StreamRole::Viewfinder, and\n> > still1Stream & still2Stream support only StreamRole::StillCapture.\n> >\n> > However, what I mean here is that when an app validates/configures\n> > a StreamConfiguration, which was updated from StreamRole::A, it\n> > should be assigned to a stream that supports StreamRole::A.\n> > (Unless the app updates its fields to be totally different.)\n> >\n> > > > Maybe libcamera core libraries assume each ISP pipe has different\n> > > > characteristics, like PixelFormat?\n> > >\n> > > I presume so.\n> > >\n> > > > > > It doesn't have to be `StreamRole`. If there are other types/arguments in\n> > > > > > the new reworked API, I'd be happy to adapt to the new one(s).\n> > > > > >\n> > > > > > Two naive ideas:\n> > > > > > 1. Keep the new `StreamRole role;` to be a const member variable in\n> > > > > > StreamConfiguration, which should be set in\n> > > > > > `PipelineHandler::generateConfiguration()`.\n> > > > > >\n> > > > > > 2. If it makes sense, add a private class for `StreamConfiguration` and keep\n> > > > > > the new `StreamRole role;` there, so that applications cannot get the info.\n> > > > > > (I don't think we need to hide the info from the applications though...)\n> > > > > >\n> > > > > > WDYT?\n> > > > >\n> > > > > Given the above described use case is my opinion valid, I wouldn't be\n> > > > > opposed to have roles as a public member of the StreamConfiguration to\n> > > > > allow application to hint how a stream should be used if no other\n> > > > > criteria is applicable.\n> > > >\n> > > > Yeah I also think that the current patch is the simplest.\n> > > >\n> > > > > > > Cc-Laurent for opinions.\n> > > > >\n> > > > > As this is an application-facing API change, I would however like to\n> > > > > see more opinions.\n> > > >\n> > > > Sure, let's wait for more opinions from others :)\n> > >\n> > > I'll make sure to discuss it with them in the next days\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n\n\n\n--\nBR,\nHarvey Yang","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 630CFC3257\n\tfor ;\n\tFri, 27 Sep 2024 16:36:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6519C6350F;\n\tFri, 27 Sep 2024 18:36:01 +0200 (CEST)","from mail-wm1-x332.google.com (mail-wm1-x332.google.com\n\t[IPv6:2a00:1450:4864:20::332])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 07EC0634F2\n\tfor ;\n\tFri, 27 Sep 2024 18:36:00 +0200 (CEST)","by mail-wm1-x332.google.com with SMTP id\n\t5b1f17b1804b1-42cae4ead5bso247965e9.1\n\tfor ;\n\tFri, 27 Sep 2024 09:35:59 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"GfIZ6rGj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=google.com; s=20230601; t=1727454959; x=1728059759;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=VRhFSiS+EosXomUdPVylSCSiS/QEcxv1R6zU6+yjdLw=;\n\tb=GfIZ6rGju4/wbIu1hzFcPJKy8TPOpLhb0YRxwvJG+wFPtLM82Fw11PMJ4HODCzjXvL\n\t2K8ZdYeTsvIKgEinmjJrzCQ8cFFuZ+bql1ZElBlQoZIhXULLslhqVDxIndfBQtcXewBV\n\t0kq/SbQFwz5dwJCO7H901glmPZ0KGfTUo+anVhM7ly1jlwxx1+g8KmxH6j0JUJ/dwDmL\n\tRfTMKzTW2hiTTfniSDAI2wDE3tfR9Tmczt1uY2qqfOj0D9AB5QNTnexKDCaIfIoMtzf1\n\tQx6vm8L7bBwD+XBSsAzS/FTpRJcYzPo56ErKdE8XM88mQT7+RL+GKFB9jS2oymhCo+b9\n\txObQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1727454959; x=1728059759;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=VRhFSiS+EosXomUdPVylSCSiS/QEcxv1R6zU6+yjdLw=;\n\tb=YaIy9HIyvUWuQf/pRmlP/Ax9y4ju8pcf2pHNVsN4RgSugKPzSOvdlqxcrRANNeMtMm\n\tJLhmymBsrMWqBlVuTxk0k9fYlrzabPsRcgaJ4KRFzEONTaZ7xOjegaamaNKAg/zeubFD\n\tJME0ZIkBTxM/NExvAV1nwg97ve0wSxYurPor8A8apse7VsVI1Y1Pi882QBFzHOl7t3Gc\n\to+tj8gMnmAO1C3XML3k2OOzo4p1IjkX+COKRVseHw3recyhvp9Cayei7AAaUY+JAZ6HL\n\t6HY3SkqyblOILwieCOo29acXzz/yLS2BYFOTged9yZHG3t9gioz5e/HyDk+UJO2N/G+B\n\t5nVA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVklhz12hiIXUW/1PGZ+Uxy+uYFuYidswYvktfwFZXR7msM3UwJBn4TZmNX/TCuBIW+LqDuDu6Msp3l2IdMwfM=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YyysHu7V3kfAxZlQvqR6grt6ZQUaqiEsxmAJ4Zlcfa66lLLNa14\n\tX1CYBuayWzpL/vi4I91YBxArLzPdaaOEWWoiVRJTRNyX+nQidb9mQDemSnOPiqKDJOP/qwCoxAB\n\twJdRZt1PJ1X7+55xY+xz2J6rTQ8mzcRE80THyzyEkTx/4SD7Njam6","X-Google-Smtp-Source":"AGHT+IEqsHmU+eGzIhk4UupW6QeJfWf3qTkUNQyIBkKeHiKdUqW0lvDW3LcDdxPktR/JM3LgOWmLnzSpMRpumR7vNoI=","X-Received":"by 2002:a05:600c:4e44:b0:42c:acd7:b59b with SMTP id\n\t5b1f17b1804b1-42f590a7c8fmr5155985e9.6.1727454959085; Fri, 27 Sep 2024\n\t09:35:59 -0700 (PDT)","MIME-Version":"1.0","References":"\n\t\n\t\n\t\n\t\n\t\n\t<5imcxykorjceprfzhynsijjian7p55fn5vry6ogfajsanczian@xtg6ooszbxo3>\n\t\n\t\n\t\n\t<20240926231346.GM21788@pendragon.ideasonboard.com>","In-Reply-To":"<20240926231346.GM21788@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Sat, 28 Sep 2024 00:35:22 +0800","Message-ID":"","Subject":"Re: [PATCH 0/1] Add StreamRole into StreamConfiguration","To":"Laurent Pinchart ","Cc":"Cheng-Hao Yang ,\n\tJacopo Mondi , \n\tkieran bingham ,\n\tlibcamera-devel@lists.libcamera.org, \n\tYaya Chang ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]