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