[0/1] Add StreamRole into StreamConfiguration

21278 mbox series

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.

This patch allows application to set the desired StreamRole directly.

This patch passed gitlab pipeline:
https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1271770

It might be controversial. Let me know the concerns. Thanks!

BR,
Harvey



Han-Lin Chen (1):
  libcamera: Add StreamRole into StreamConfiguration

 include/libcamera/stream.h |  2 ++
 src/libcamera/stream.cpp   | 12 ++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

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 ?

>
> This patch allows application to set the desired StreamRole directly.
>
> This patch passed gitlab pipeline:
> https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1271770
>
> It might be controversial. Let me know the concerns. Thanks!
>

The fact is that roles are designed to hint libcamera about how to
populate the CameraConfiguration, after that point they have no
meaning anymore for the library.

I presum you need this for Android, right ? Isn't it better to keep
the association between the Android role and the libcamera Stream in
the Android HAL instead ? Be aware that the CameraConfiguration should
be populated with StreamConfigurations in the same order as the
StreamRole order the application asked for.

If instead we want to keep the Role in Stream, it shouldn't be up to
the application to populate it, but it should be the pipeline's
generateConfiguration() function that should do this probably.

Thanks
  j

> BR,
> Harvey
>
>
>
> Han-Lin Chen (1):
>   libcamera: Add StreamRole into StreamConfiguration
>
>  include/libcamera/stream.h |  2 ++
>  src/libcamera/stream.cpp   | 12 ++++++++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> --
> 2.46.0.662.g92d0881bb0-goog
>

Hi Jacopo,

On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi <jacopo.mondi@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 ?
>
> >
> > This patch allows application to set the desired StreamRole directly.
> >
> > This patch passed gitlab pipeline:
> >
> https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1271770
> >
> > It might be controversial. Let me know the concerns. Thanks!
> >
>
> The fact is that roles are designed to hint libcamera about how to
> populate the CameraConfiguration, after that point they have no
> meaning anymore for the library.
>
> I presum you need this for Android, right ?


Correct :)


> Isn't it better to keep
> the association between the Android role and the libcamera Stream in
> the Android HAL instead ?


Sorry, I don't quite get your point. What's the Android role?
When doing `validate()` or `configure()`, how does Android HAL/adapter let
Pipeline Handler know which StreamConfiguration the HAL desires it to
belong to?


> Be aware that the CameraConfiguration should
> be populated with StreamConfigurations in the same order as the
> StreamRole order the application asked for.
>

Yes, I remember that.


>
> If instead we want to keep the Role in Stream, it shouldn't be up to
> the application to populate it, but it should be the pipeline's
> generateConfiguration() function that should do this probably.
>
>
Yes, actually in mtkisp7's implementation, that's how we do it [1].
Basically Android HAL/adapter will not change the StreamRole in
StreamConfiguration, while IIUC the API is designed in the way that
applications can change any attribute in a StreamConfiguration,
before calling `validate()` or `configuration()`, and
`Camera::generateConfiguration()` only provides the initial/suggested
configuration, right?

[1]:
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=563

BR,
Harvey

Thanks
>   j
>
> > BR,
> > Harvey
> >
> >
> >
> > Han-Lin Chen (1):
> >   libcamera: Add StreamRole into StreamConfiguration
> >
> >  include/libcamera/stream.h |  2 ++
> >  src/libcamera/stream.cpp   | 12 ++++++++++--
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > --
> > 2.46.0.662.g92d0881bb0-goog
> >
>

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

Sorry Jacopo, my bad to miss the first message:

On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi <jacopo.mondi@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@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.

Does that make sense :P?

BR,
Harvey

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@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@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 ? Does
the mtkisp7 pipes have different capabilities when it comes to
supported formats and resolutions ?

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

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.

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 ?

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.

Cc-Laurent for opinions.

>
> BR,
> Harvey

Hi Jacopo and Laurent,

On Thu, Sep 19, 2024 at 5:09 PM Jacopo Mondi <jacopo.mondi@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@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@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
>

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

Thanks Jacopo for looking into the tree of the mtkisp7 branch!


On Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com>
wrote:

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

Yes, that's a great idea to avoid confusion.


>
> >
> > [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".
>

Yes, that's the main concern :)


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

I think that's also related to another confusion that I have regarding the
current libcamera API: We use `StreamRole` as the input to get the
default configuration, while it doesn't stop a pipeline handler to assign
it to a different stream, with a different StreamRole, later in `validate()`
or `configure()`. The pipeline handler might not even return
`Status::Adjusted`, if the requested arguments are not changed.

Maybe libcamera core libraries assume each ISP pipe has different
characteristics, like PixelFormat?


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

Yeah I also think that the current patch is the simplest.


>
> >
> >
> > > Cc-Laurent for opinions.
>
> As this is an application-facing API change, I would however like to
> see more opinions.
>

Sure, let's wait for more opinions from others :)


>
> Thanks
>   j
>
> > >
> > > >
> > > > BR,
> > > > Harvey
> > >
> >
> >
> > --
> > BR,
> > Harvey Yang
>

Hi Harvey

On Mon, Sep 23, 2024 at 10:14:48PM GMT, Cheng-Hao Yang wrote:
> Thanks Jacopo for looking into the tree of the mtkisp7 branch!
>
>
> On Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> wrote:
>
> > 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@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@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@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.
> >
>
> Yes, that's a great idea to avoid confusion.
>
>
> >
> > >
> > > [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".
> >
>
> Yes, that's the main concern :)
>
>
> >
> > 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.
> >
>
> I think that's also related to another confusion that I have regarding the
> current libcamera API: We use `StreamRole` as the input to get the
> default configuration, while it doesn't stop a pipeline handler to assign
> it to a different stream, with a different StreamRole, later in `validate()`

Which makes me wonder, if we allow apps to set StreamConfig::role, how
are we going to validate it ?

> or `configure()`. The pipeline handler might not even return
> `Status::Adjusted`, if the requested arguments are not changed.

The thing is that StreamRoles usage should have been limited to
generateConfiguration() only. It shouldn't be related to which Stream
in the pipeline handler a StreamConfig is assigned to, as there's no
1-to-1 matching between StreamRoles (libcamera API) and the number and
charateristics of a Stream (platform specific).

>
> Maybe libcamera core libraries assume each ISP pipe has different
> characteristics, like PixelFormat?
>

I presume so.

>
> >
> >
> > > 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.
> >
>
> Yeah I also think that the current patch is the simplest.
>
>
> >
> > >
> > >
> > > > Cc-Laurent for opinions.
> >
> > As this is an application-facing API change, I would however like to
> > see more opinions.
> >
>
> Sure, let's wait for more opinions from others :)
>

I'll make sure to discuss it  with them in the next days

>
> >
> > Thanks
> >   j
> >
> > > >
> > > > >
> > > > > BR,
> > > > > Harvey
> > > >
> > >
> > >
> > > --
> > > BR,
> > > Harvey Yang
> >
>
>
> --
> BR,
> Harvey Yang

Hi,

Le lundi 23 septembre 2024 à 22:14 +0800, Cheng-Hao Yang a écrit :
> Thanks Jacopo for looking into the tree of the mtkisp7 branch!

Small technical note, it will be appreciated by many users if you reply in text
form. This is true for most mailing list out there.

Nicolas

> 
> 
> On Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> > 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@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@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@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.
> > 
> 
> 
> Yes, that's a great idea to avoid confusion.
>  
> > 
> > > 
> > > [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".
> > 
> 
> 
> Yes, that's the main concern :)
>  
> > 
> > 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.
> > 
> 
> 
> I think that's also related to another confusion that I have regarding the
> current libcamera API: We use `StreamRole` as the input to get the
> default configuration, while it doesn't stop a pipeline handler to assign
> it to a different stream, with a different StreamRole, later in `validate()`
> or `configure()`. The pipeline handler might not even return
> `Status::Adjusted`, if the requested arguments are not changed.
> 
> Maybe libcamera core libraries assume each ISP pipe has different
> characteristics, like PixelFormat?
>  
> > 
> > 
> > > 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.
> > 
> 
> 
> Yeah I also think that the current patch is the simplest.
>  
> > 
> > > 
> > > 
> > > > Cc-Laurent for opinions.
> > 
> > As this is an application-facing API change, I would however like to
> > see more opinions.
> > 
> 
> 
> Sure, let's wait for more opinions from others :)
>  
> > 
> > Thanks
> >   j
> > 
> > > > 
> > > > > 
> > > > > BR,
> > > > > Harvey
> > > > 
> > > 
> > > 
> > > --
> > > BR,
> > > Harvey Yang
> 
> 
> -- 
> BR,
> Harvey Yang

On Tue, Sep 24, 2024 at 2:36 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
>
> Hi,
>
> Le lundi 23 septembre 2024 à 22:14 +0800, Cheng-Hao Yang a écrit :
> > Thanks Jacopo for looking into the tree of the mtkisp7 branch!
>
> Small technical note, it will be appreciated by many users if you reply in text
> form. This is true for most mailing list out there.

Ah okay. Hope I'm doing it right this way.


>
>
> Nicolas
>
> >
> >
> > On Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> > > 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@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@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@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.
> > >
> >
> >
> > Yes, that's a great idea to avoid confusion.
> >
> > >
> > > >
> > > > [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".
> > >
> >
> >
> > Yes, that's the main concern :)
> >
> > >
> > > 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.
> > >
> >
> >
> > I think that's also related to another confusion that I have regarding the
> > current libcamera API: We use `StreamRole` as the input to get the
> > default configuration, while it doesn't stop a pipeline handler to assign
> > it to a different stream, with a different StreamRole, later in `validate()`
> > or `configure()`. The pipeline handler might not even return
> > `Status::Adjusted`, if the requested arguments are not changed.
> >
> > Maybe libcamera core libraries assume each ISP pipe has different
> > characteristics, like PixelFormat?
> >
> > >
> > >
> > > > 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.
> > >
> >
> >
> > Yeah I also think that the current patch is the simplest.
> >
> > >
> > > >
> > > >
> > > > > Cc-Laurent for opinions.
> > >
> > > As this is an application-facing API change, I would however like to
> > > see more opinions.
> > >
> >
> >
> > Sure, let's wait for more opinions from others :)
> >
> > >
> > > Thanks
> > >   j
> > >
> > > > >
> > > > > >
> > > > > > BR,
> > > > > > Harvey
> > > > >
> > > >
> > > >
> > > > --
> > > > BR,
> > > > Harvey Yang
> >
> >
> > --
> > BR,
> > Harvey Yang
>

Hi Jacopo,

On Mon, Sep 23, 2024 at 11:10 PM Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Harvey
>
> On Mon, Sep 23, 2024 at 10:14:48PM GMT, Cheng-Hao Yang wrote:
> > Thanks Jacopo for looking into the tree of the mtkisp7 branch!
> >
> >
> > On Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > wrote:
> >
> > > 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@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@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@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.
> > >
> >
> > Yes, that's a great idea to avoid confusion.
> >
> >
> > >
> > > >
> > > > [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".
> > >
> >
> > Yes, that's the main concern :)
> >
> >
> > >
> > > 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.
> > >
> >
> > I think that's also related to another confusion that I have regarding the
> > current libcamera API: We use `StreamRole` as the input to get the
> > default configuration, while it doesn't stop a pipeline handler to assign
> > it to a different stream, with a different StreamRole, later in `validate()`
>
> Which makes me wonder, if we allow apps to set StreamConfig::role, how
> are we going to validate it ?

I assume you mean that if a StreamConfiguration contains StreamRole
along with other fields, and they conflict with each other with apps'
manipulation, how can the pipeline handler validate it?
In this case, I think the pipeline handler should return Invalid directly,
or fix the StreamConfiguration properly and return Adjusted.

>
> > or `configure()`. The pipeline handler might not even return
> > `Status::Adjusted`, if the requested arguments are not changed.
>
> The thing is that StreamRoles usage should have been limited to
> generateConfiguration() only. It shouldn't be related to which Stream
> in the pipeline handler a StreamConfig is assigned to, as there's no
> 1-to-1 matching between StreamRoles (libcamera API) and the number and
> charateristics of a Stream (platform specific).

I agree that the 1-to-1 matching doesn't exist. Take mtkisp7 as the
example, video1Stream_ & video2Stream_ can support both
StreamRole::VideoRecording & StreamRole::Viewfinder, and
still1Stream & still2Stream support only StreamRole::StillCapture.

However, what I mean here is that when an app validates/configures
a StreamConfiguration, which was updated from StreamRole::A, it
should be assigned to a stream that supports StreamRole::A.
(Unless the app updates its fields to be totally different.)

BR,
Harvey

>
> >
> > Maybe libcamera core libraries assume each ISP pipe has different
> > characteristics, like PixelFormat?
> >
>
> I presume so.
>
> >
> > >
> > >
> > > > 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.
> > >
> >
> > Yeah I also think that the current patch is the simplest.
> >
> >
> > >
> > > >
> > > >
> > > > > Cc-Laurent for opinions.
> > >
> > > As this is an application-facing API change, I would however like to
> > > see more opinions.
> > >
> >
> > Sure, let's wait for more opinions from others :)
> >
>
> I'll make sure to discuss it  with them in the next days
>
> >
> > >
> > > Thanks
> > >   j
> > >
> > > > >
> > > > > >
> > > > > > BR,
> > > > > > Harvey
> > > > >
> > > >
> > > >
> > > > --
> > > > BR,
> > > > Harvey Yang
> > >
> >
> >
> > --
> > BR,
> > Harvey Yang

Le mardi 24 septembre 2024 à 13:29 +0800, Cheng-Hao Yang a écrit :
> On Tue, Sep 24, 2024 at 2:36 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > 
> > 
> > Hi,
> > 
> > Le lundi 23 septembre 2024 à 22:14 +0800, Cheng-Hao Yang a écrit :
> > > Thanks Jacopo for looking into the tree of the mtkisp7 branch!
> > 
> > Small technical note, it will be appreciated by many users if you reply in text
> > form. This is true for most mailing list out there.
> 
> Ah okay. Hope I'm doing it right this way.

Thank you, this is exactly what I meant.

Nicolas

[...]

Hi Harvey,

I finally managed to go through the conversation. I'll top-post as
there's one question I haven't seen being discussed.

The mail thread has focussed on the mtkisp7, whose outputs all support
the same formats and are therefore not distinguishable from each other
with the current API. You've proposed using stream roles to
differentiate between the outputs. Before discussing what the right
solution is, I'd like to understand how the outputs differ from each
other. Surely if they all had the exact same capabilities we wouldn't
have this conversation, so how do they differ ? I think understanding
this will be key to designing the right API.

On Tue, Sep 24, 2024 at 02:42:43PM +0800, Cheng-Hao Yang wrote:
> On Mon, Sep 23, 2024 at 11:10 PM Jacopo Mondi wrote:
> > On Mon, Sep 23, 2024 at 10:14:48PM GMT, Cheng-Hao Yang wrote:
> > > On Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi wrote:
> > > > On Thu, Sep 19, 2024 at 09:55:58PM GMT, Cheng-Hao Yang wrote:
> > > > > On Thu, Sep 19, 2024 at 5:09 PM Jacopo Mondi wrote:
> > > > > > On Wed, Sep 18, 2024 at 04:33:27PM GMT, Cheng-Hao Yang wrote:
> > > > > > > On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi wrote:
> > > > > > > > On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:
> > > > > > > > > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi wrote:
> > > > > > > > > > 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.
> > >
> > > Yes, that's a great idea to avoid confusion.
> > >
> > > > > [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".
> > >
> > > Yes, that's the main concern :)
> > >
> > > > 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.
> > > >
> > >
> > > I think that's also related to another confusion that I have regarding the
> > > current libcamera API: We use `StreamRole` as the input to get the
> > > default configuration, while it doesn't stop a pipeline handler to assign
> > > it to a different stream, with a different StreamRole, later in `validate()`
> >
> > Which makes me wonder, if we allow apps to set StreamConfig::role, how
> > are we going to validate it ?
> 
> I assume you mean that if a StreamConfiguration contains StreamRole
> along with other fields, and they conflict with each other with apps'
> manipulation, how can the pipeline handler validate it?
> In this case, I think the pipeline handler should return Invalid directly,
> or fix the StreamConfiguration properly and return Adjusted.
> 
> > > or `configure()`. The pipeline handler might not even return
> > > `Status::Adjusted`, if the requested arguments are not changed.
> >
> > The thing is that StreamRoles usage should have been limited to
> > generateConfiguration() only. It shouldn't be related to which Stream
> > in the pipeline handler a StreamConfig is assigned to, as there's no
> > 1-to-1 matching between StreamRoles (libcamera API) and the number and
> > charateristics of a Stream (platform specific).
> 
> I agree that the 1-to-1 matching doesn't exist. Take mtkisp7 as the
> example, video1Stream_ & video2Stream_ can support both
> StreamRole::VideoRecording & StreamRole::Viewfinder, and
> still1Stream & still2Stream support only StreamRole::StillCapture.
> 
> However, what I mean here is that when an app validates/configures
> a StreamConfiguration, which was updated from StreamRole::A, it
> should be assigned to a stream that supports StreamRole::A.
> (Unless the app updates its fields to be totally different.)
> 
> > > Maybe libcamera core libraries assume each ISP pipe has different
> > > characteristics, like PixelFormat?
> >
> > I presume so.
> >
> > > > > 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.
> > >
> > > Yeah I also think that the current patch is the simplest.
> > >
> > > > > > Cc-Laurent for opinions.
> > > >
> > > > As this is an application-facing API change, I would however like to
> > > > see more opinions.
> > >
> > > Sure, let's wait for more opinions from others :)
> >
> > I'll make sure to discuss it  with them in the next days

Hi Laurent,

On Fri, Sep 27, 2024 at 7:13 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Harvey,
>
> I finally managed to go through the conversation. I'll top-post as
> there's one question I haven't seen being discussed.
>
> The mail thread has focussed on the mtkisp7, whose outputs all support
> the same formats and are therefore not distinguishable from each other
> with the current API. You've proposed using stream roles to
> differentiate between the outputs. Before discussing what the right
> solution is, I'd like to understand how the outputs differ from each
> other. Surely if they all had the exact same capabilities we wouldn't
> have this conversation, so how do they differ ? I think understanding
> this will be key to designing the right API.

I agree that understanding how the outputs differ from each other is
one of the keys.
However in mtkisp7, I've confirmed with Han-lin and Yaya from
MediaTek that users/applications can ask for two streams, one Video
and one StillCapture, with identical characteristics (in terms of the
current StreamConfiguration fields [1]), and with only Image Quality
differences, as it goes to different noise reduction pipes in the ISP.

Regarding StreamConfiguration fields, we find `PixelFormat`, `Size`,
`ColorSpace`, and `bufferCount` to be available for pipeline handlers to
distinguish stream configurations, while these three can be identical
for Video and StillCapture streams in mtkisp7. `stride` and `frameSize`
should be set by pipeline handlers.

Therefore, we couldn't find the fields to help distinguish the stream
configurations for mtkisp7.

In mtkisp7, the preview/video streams go through Motion-Compensated
Noise Reduction, and still capture streams go through Low-Pass Noise
Reduction or Multi-Frame Noise Reduction. They also might load
different tuning parameters, while they should be agnostic to the apps.

[1]: https://git.libcamera.org/libcamera/libcamera.git/tree/include/libcamera/stream.h#n40


>
> On Tue, Sep 24, 2024 at 02:42:43PM +0800, Cheng-Hao Yang wrote:
> > On Mon, Sep 23, 2024 at 11:10 PM Jacopo Mondi wrote:
> > > On Mon, Sep 23, 2024 at 10:14:48PM GMT, Cheng-Hao Yang wrote:
> > > > On Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi wrote:
> > > > > On Thu, Sep 19, 2024 at 09:55:58PM GMT, Cheng-Hao Yang wrote:
> > > > > > On Thu, Sep 19, 2024 at 5:09 PM Jacopo Mondi wrote:
> > > > > > > On Wed, Sep 18, 2024 at 04:33:27PM GMT, Cheng-Hao Yang wrote:
> > > > > > > > On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi wrote:
> > > > > > > > > On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:
> > > > > > > > > > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi wrote:
> > > > > > > > > > > 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.
> > > >
> > > > Yes, that's a great idea to avoid confusion.
> > > >
> > > > > > [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 :)

Let me correct myself: I find that Android adapter sets
StreamConfigurations from
scratch [2]. It means that Android adapter cares only about
pixelFormat and size.
Maybe it's not sufficient, and we can update it to use the default ones fetched
before.

It doesn't change the story though.

[2]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n638

> > > > >
> > > > > 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".
> > > >
> > > > Yes, that's the main concern :)
> > > >
> > > > > 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.
> > > > >
> > > >
> > > > I think that's also related to another confusion that I have regarding the
> > > > current libcamera API: We use `StreamRole` as the input to get the
> > > > default configuration, while it doesn't stop a pipeline handler to assign
> > > > it to a different stream, with a different StreamRole, later in `validate()`
> > >
> > > Which makes me wonder, if we allow apps to set StreamConfig::role, how
> > > are we going to validate it ?
> >
> > I assume you mean that if a StreamConfiguration contains StreamRole
> > along with other fields, and they conflict with each other with apps'
> > manipulation, how can the pipeline handler validate it?
> > In this case, I think the pipeline handler should return Invalid directly,
> > or fix the StreamConfiguration properly and return Adjusted.
> >
> > > > or `configure()`. The pipeline handler might not even return
> > > > `Status::Adjusted`, if the requested arguments are not changed.
> > >
> > > The thing is that StreamRoles usage should have been limited to
> > > generateConfiguration() only. It shouldn't be related to which Stream
> > > in the pipeline handler a StreamConfig is assigned to, as there's no
> > > 1-to-1 matching between StreamRoles (libcamera API) and the number and
> > > charateristics of a Stream (platform specific).
> >
> > I agree that the 1-to-1 matching doesn't exist. Take mtkisp7 as the
> > example, video1Stream_ & video2Stream_ can support both
> > StreamRole::VideoRecording & StreamRole::Viewfinder, and
> > still1Stream & still2Stream support only StreamRole::StillCapture.
> >
> > However, what I mean here is that when an app validates/configures
> > a StreamConfiguration, which was updated from StreamRole::A, it
> > should be assigned to a stream that supports StreamRole::A.
> > (Unless the app updates its fields to be totally different.)
> >
> > > > Maybe libcamera core libraries assume each ISP pipe has different
> > > > characteristics, like PixelFormat?
> > >
> > > I presume so.
> > >
> > > > > > 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.
> > > >
> > > > Yeah I also think that the current patch is the simplest.
> > > >
> > > > > > > Cc-Laurent for opinions.
> > > > >
> > > > > As this is an application-facing API change, I would however like to
> > > > > see more opinions.
> > > >
> > > > Sure, let's wait for more opinions from others :)
> > >
> > > I'll make sure to discuss it  with them in the next days
>
> --
> Regards,
>
> Laurent Pinchart



--
BR,
Harvey Yang