[PATCH 0/1] Add StreamRole into StreamConfiguration

[Cheng-Hao Yang]

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].

[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 :)

[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

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 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?


> Cc-Laurent for opinions.
>
> >
> > BR,
> > Harvey
>


-- 
BR,
Harvey Yang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240919/9b6ebc98/attachment.htm>