<div dir="ltr"><div dir="ltr">Thanks Jacopo for looking into the tree of the mtkisp7 branch!<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com">jacopo.mondi@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Harvey<br>
<br>
On Thu, Sep 19, 2024 at 09:55:58PM GMT, Cheng-Hao Yang wrote:<br>
> Hi Jacopo and Laurent,<br>
><br>
> On Thu, Sep 19, 2024 at 5:09 PM Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> wrote:<br>
><br>
> > Hi Harvey<br>
> ><br>
> > On Wed, Sep 18, 2024 at 04:33:27PM GMT, Cheng-Hao Yang wrote:<br>
> > > Sorry Jacopo, my bad to miss the first message:<br>
> > ><br>
> > > On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi <<br>
> > <a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> > > wrote:<br>
> > ><br>
> > > > Hi Harvey<br>
> > > ><br>
> > > > On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:<br>
> > > > > Hi Jacopo,<br>
> > > > ><br>
> > > > > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi <<br>
> > > > <a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> > > > > wrote:<br>
> > > > ><br>
> > > > > > Hi Harvey<br>
> > > > > ><br>
> > > > > > On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:<br>
> > > > > > > Hi folks,<br>
> > > > > > ><br>
> > > > > > > Currently applications set resolutions, pixelFormat, bufferCount,<br>
> > > > etc,<br>
> > > > > > > into StreamConfigurations, and Pipeline Handler decides which<br>
> > streams<br>
> > > > > > > they're assigned to. However, it doesn't allow application to<br>
> > assign<br>
> > > > > > > streams that cannot be distinguished by those arguments into<br>
> > > > > > > VideoRecording or StillCapture (say YUV/NV12 format), which is<br>
> > > > needed in<br>
> > > > > > > mtkisp7.<br>
> > > > > ><br>
> > > > > > Could you explain in a bit more detail why this "is needed" and how<br>
> > > > > > you plan to use StreamRole as part of the stream configuration ?<br>
> > > > > ><br>
> > > ><br>
> > > > Could you explain in a bit more detail why this "is needed" and how<br>
> > > > you plan to use StreamRole as part of the stream configuration ?<br>
> > > ><br>
> > > ><br>
> > > ><br>
> > > In mtkisp7 pipeline handler, both preview (or video) and still capture<br>
> > > streams support the same format (NV12) and bufferCount, and the<br>
> > > maximum resolutions are also the same. Therefore, when calling<br>
> > > `validate()` and `configure()`, mtkisp7 pipeline handler doesn't know<br>
> > > if applications/Android adapter wants the stream to go through the<br>
> > > still captur pipeline or not.<br>
> ><br>
> > I still have an hard time understanding why validate() and configure()<br>
> > needs to know the "role". Is this to assign "pipes" to Streams ?<br>
><br>
><br>
> If I understand correctly that "pipes" means the pipelines of ISP/IPA algos<br>
> for preview/video/still capture respectively, yes, it's to assign the<br>
> StreamConfiguration(s) to the desired pipe(s), which means to call<br>
> `StreamConfiguration::setStream()`. According to the comments [1], it's<br>
> supposed to be called in `PipelineHandler::configure()`, like<br>
> SimplePipelineHandler [2]. However, there are also some pipeline handlers<br>
> that set them in `CameraConfiguration::validate()` [3] [4].<br>
<br>
Indeed most of our documentation suggests that setStream() is meant to<br>
be called during configure(). Considering that validate() is -always-<br>
called before configure() by Camera, so where exactly you call it, I<br>
don't think makes a big difference.<br>
<br>
We could also relax the documentation.<br></blockquote><div><br></div><div>Yes, that's a great idea to avoid confusion.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
> [1]:<br>
> <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n303" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n303</a><br>
> [2]:<br>
> <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/simple/simple.cpp#n1270" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/simple/simple.cpp#n1270</a><br>
> [3]:<br>
> <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n347" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n347</a><br>
> [4]:<br>
> <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rkisp1/rkisp1.cpp#n520" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rkisp1/rkisp1.cpp#n520</a><br>
><br>
> Does<br>
> > the mtkisp7 pipes have different capabilities when it comes to<br>
> > supported formats and resolutions ?<br>
> ><br>
><br>
> No, as you can see in mtkisp7's implementation [5], all of them support the<br>
> same formats and resolutions.<br>
><br>
> [5]:<br>
> <a href="https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556" rel="noreferrer" target="_blank">https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556</a><br>
><br>
><br>
><br>
><br>
> > ><br>
> > > Does that make sense :P?<br>
> ><br>
> > I guess so, but I wonder if the "role" is the main criteria that<br>
> > should be taken into account when assiging pipes to streams.<br>
> ><br>
> How would this work for applications that operates on platforms where<br>
> > the pipe selection depends on other criteria like the supported image<br>
> > formats and stream resolutions ? In example, for rkisp1<br>
> > the "self" pipelines can do RGB while the "main" can't and that's how<br>
> > we assign pipes during validate().<br>
><br>
><br>
> > To be honest this has not proven to be optimal and I'm not opposed to<br>
> > add Roles to the pipe selection criteria, but they shouldn't be made the<br>
> > only criteria on which pipes are assigned (unless we support this on<br>
> > all pipelines).<br>
> ><br>
> ><br>
> I agree that the `role` might not be the main criteria in general cases.<br>
> It's just that we find difficulties in assigning them properly with the<br>
> current<br>
> configurations in mtkisp7.<br>
><br>
><br>
><br>
> > Also I would not based any future-proof design on Roles, they will<br>
> > likely be heavily reworked or removed going forward.<br>
> ><br>
> > As your main use case is Android, I think it would be doable for you<br>
> > to<br>
> > 1) Request a generateConfiguration() and keep track of the<br>
> > StreamConfiguration order.<br>
> ><br>
> > generateConfiguration(Viewfinder, Still, Raw)<br>
> ><br>
> > will give you StreamConfiguration for the above rols in order as<br>
> > you have requested them, if supported.<br>
> ><br>
> > 2) Code validate() so that you try to assing pipes based on the<br>
> > formats/sizes and if formats/sizes are the same define an ordering.<br>
> > In example the first YUV/WxH stream goes to Viewfinder if an<br>
> > identical YUV/WxH stream is requested for Still Capture.<br>
> ><br>
><br>
> This actually assumes the application doesn't change the content of the<br>
> default CameraConfiguration/StreamConfiguration, which I doubt if it's a<br>
> good thing. Currently in Android adapter, it re-arranges [6]<br>
> StreamConfigurations based on the previously-fetched default<br>
> StreamConfigurations of the StreamRoles, which I think is a normal<br>
> practice of the current interfaces' logic. Please correct me if I'm wrong :)<br>
<br>
Good point, as Android HAL re-sorts the StreamConfiguration before<br>
calling Camera::configure() you can't rely on the order in which you<br>
have requested roles to Camera::generateConfiguration().<br>
<br>
><br>
> [6]:<br>
> <a href="https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n708" rel="noreferrer" target="_blank">https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n708</a><br>
><br>
> Also, if an application calls<br>
> `generateConfiguration(camera, {StreamRole::VideoRecording})` and<br>
> `generateConfiguration(camera, {StreamRole::StillCapture})`<br>
> respectively, and ends up calling `configure()` with `VideoRecording`'s<br>
> CameraConfiguration, how would the pipeline handler know that the<br>
> application intends to get buffers from the stream `VideoRecording`?<br>
> In your suggestion, it seems that the application needs to call<br>
> `generateConfiguration(camera, {StreamRole::VideoRecording})` again,<br>
> which I don't think is enforced in the current libcamera API, and might<br>
> not be a good practice.<br>
><br>
><br>
> > 3) As validate() assigns Steams * to StreamConfiguration (with<br>
> > ::setStream) it should be easy to keep track to which pipe a<br>
> > StreamConfiguration has been assigned to at configure() time.<br>
> ><br>
> > Could you list what are the platform's pipes capabilities ?<br>
> ><br>
> ><br>
> For example, on `ciri` with mtkisp7 pipeline handler [5], `pixelFormat` is<br>
> NV12,<br>
> supported resolution range is `320x240` to `2592x1944`, and `bufferCount` is<br>
> 8. It applies to `StillCapture`, `VideoRecording`, and `ViewFinder`.<br>
><br>
> [5]:<br>
> <a href="https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556" rel="noreferrer" target="_blank">https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556</a><br>
><br>
<br>
Looking at the pipeline handler implementation from your tree it seems<br>
to me that you're using roles in validate() to call setStream(), as we<br>
already clarified:<br>
<br>
switch (cfg.role) {<br>
case StreamRole::Viewfinder:<br>
case StreamRole::VideoRecording:<br>
if (videoCnt >= 2) {<br>
LOG(MtkISP7, Error)<br>
<< "Support only 2 Preview/Video streams";<br>
return Invalid;<br>
}<br>
cfg.setStream(const_cast<Stream *>(vidStreams[videoCnt++]));<br>
break;<br>
case StreamRole::StillCapture:<br>
if (stillCnt >= 2) {<br>
LOG(MtkISP7, Error)<br>
<< "Support only 2 StillCapture streams";<br>
return Invalid;<br>
}<br>
cfg.setStream(const_cast<Stream *>(stillStreams[stillCnt++]));<br>
break;<br>
default:<br>
LOG(MtkISP7, Error) << "Invalid StreamRole " << cfg.role;<br>
return Invalid;<br>
}<br>
<br>
This shows that you have 4 streams, 2 for Preview/Video and 2 for<br>
StillCapture.<br>
<br>
Then you use the Stream in configure() for populate two sizes<br>
<br>
Size video1 = Size{ 0, 0 };<br>
Size video2 = Size{ 0, 0 };<br>
Size still1 = Size{ 0, 0 };<br>
Size still2 = Size{ 0, 0 };<br>
<br>
/* Only cover the video resolution */<br>
for (auto &cfg : *c) {<br>
if (cfg.stream() == &video1Stream_)<br>
video1 = cfg.size;<br>
else if (cfg.stream() == &video2Stream_)<br>
video2 = cfg.size;<br>
else if (cfg.stream() == &still1Stream_)<br>
still1 = cfg.size;<br>
else if (cfg.stream() == &still2Stream_)<br>
still2 = cfg.size;<br>
else<br>
return -EINVAL;<br>
}<br>
<br>
>From there on, all the API you have implemented relies on the presence<br>
and order of the 'video1', 'video2, 'still1' and 'still2' parameters.<br>
<br>
In example<br>
<br>
mcnrManager.configure(camsysYuvSize, video1, video2);<br>
lpnrManager.configure(sensorFullSize_, still1, still2);<br>
lpnrTunManager.configure(sensorFullSize_, still1, still2);<br>
mcnrTunManager.configure(camsysYuvSize, video1, video2);<br>
<br>
Now, according to what I've read and what you said, there are no<br>
constraints on the HW that help identify Stream. To make an example,<br>
you can't assume "Oh this StreamConfiguration is RGB so it needs to go<br>
to StreamX".<br></blockquote><div><br></div><div>Yes, that's the main concern :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
So I guess what you want is to allow an application to say "One<br>
viewfinder is WxH, one Still capture is WxH, one viewfinder is WxH"<br>
(also, all Streams are NV12 if I'm not mistaken).<br>
<br>
> As said, I'm not opposed to use Roles for assigning pipes, but I don't<br>
> > think we should based any new development on Roles as there's an high<br>
> > chance they can be reworked.<br>
> ><br>
><br>
> Could you briefly describe how the new APIs would be like?<br>
><br>
> Basically I think either applications need to have an argument to indicate<br>
> which kind of stream the StreamConfiguration needs to be assigned to, or<br>
> the pipeline handler needs to remember which StreamRole a<br>
> StreamConfiguration returned was asked as the default configuration for.<br>
<br>
It's not really about that, the idea is that the stream's<br>
characteristics should be used to assign a stream to a pipe, like the<br>
format and the sizes. In your case that's not really possible as all<br>
streams are NV12 and all resolutions can go anywhere.<br>
<br>
But, as you use roles, that logic should live somewhere, doesn't it ?<br>
<br>
Looking at your implementation of the Android HAL I see (sorry, cannot<br>
point out a commit id as there's quite some reverts in the history)<br>
<br>
if (isJpegStream(stream)) {<br>
continue;<br>
} else if (isYuvSnapshotStream(stream)) {<br>
streamConfig.streams = { { stream, CameraStream::Type::Direct } };<br>
streamConfig.config.role = StreamRole::StillCapture;<br>
} else if (isPreviewStream(stream)) {<br>
streamConfig.streams = { { stream, CameraStream::Type::Direct } };<br>
streamConfig.config.role = StreamRole::Viewfinder;<br>
} else if (isVideoStream(stream)) {<br>
streamConfig.streams = { { stream, CameraStream::Type::Direct } };<br>
streamConfig.config.role = StreamRole::VideoRecording;<br>
} else {<br>
streamConfig.streams = { { stream, CameraStream::Type::Direct } };<br>
streamConfig.config.role = StreamRole::Viewfinder;<br>
}<br>
<br>
So you populate roles based on some criteria here, and if I look at<br>
the criteria<br>
<br>
bool isPreviewStream(camera3_stream_t *stream)<br>
{<br>
return (GRALLOC_USAGE_HW_COMPOSER & stream->usage);<br>
}<br>
<br>
bool isVideoStream(camera3_stream_t *stream)<br>
{<br>
return (GRALLOC_USAGE_HW_VIDEO_ENCODER & stream->usage);<br>
}<br>
<br>
bool isYuvSnapshotStream(camera3_stream_t *stream)<br>
{<br>
return (!isVideoStream(stream) && !isPreviewStream(stream) &&<br>
(HAL_PIXEL_FORMAT_YCbCr_420_888 == stream->format));<br>
}<br>
<br>
they indeed come from Android's requirements that cannot be expressed<br>
in libcamera.<br>
<br>
To be clear, when asking "what you need this for" this is the level of<br>
detail that is needed to explain your design choices. Otherwise it's<br>
up to us to decode what you have done in the mtk support branch.<br></blockquote><div><br></div><div>I think that's also related to another confusion that I have regarding the</div><div>current libcamera API: We use `StreamRole` as the input to get the</div><div>default configuration, while it doesn't stop a pipeline handler to assign</div><div>it to a different stream, with a different StreamRole, later in `validate()`</div><div>or `configure()`. The pipeline handler might not even return</div><div>`Status::Adjusted`, if the requested arguments are not changed.</div><div><br></div><div>Maybe libcamera core libraries assume each ISP pipe has different</div><div>characteristics, like PixelFormat?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
> It doesn't have to be `StreamRole`. If there are other types/arguments in<br>
> the new reworked API, I'd be happy to adapt to the new one(s).<br>
><br>
> Two naive ideas:<br>
> 1. Keep the new `StreamRole role;` to be a const member variable in<br>
> StreamConfiguration, which should be set in<br>
> `PipelineHandler::generateConfiguration()`.<br>
><br>
> 2. If it makes sense, add a private class for `StreamConfiguration` and keep<br>
> the new `StreamRole role;` there, so that applications cannot get the info.<br>
> (I don't think we need to hide the info from the applications though...)<br>
><br>
> WDYT?<br>
<br>
Given the above described use case is my opinion valid, I wouldn't be<br>
opposed to have roles as a public member of the StreamConfiguration to<br>
allow application to hint how a stream should be used if no other<br>
criteria is applicable.<br></blockquote><div><br></div><div>Yeah I also think that the current patch is the simplest.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
><br>
><br>
> > Cc-Laurent for opinions.<br>
<br>
As this is an application-facing API change, I would however like to<br>
see more opinions.<br></blockquote><div><br></div><div>Sure, let's wait for more opinions from others :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Thanks<br>
j<br>
<br>
> ><br>
> > ><br>
> > > BR,<br>
> > > Harvey<br>
> ><br>
><br>
><br>
> --<br>
> BR,<br>
> Harvey Yang<br>
</blockquote></div><br clear="all"><div><br></div><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>BR,</div>Harvey Yang</div></div></div>