[PATCH 0/1] Add StreamRole into StreamConfiguration
Cheng-Hao Yang
chenghaoyang at google.com
Fri Sep 27 18:35:22 CEST 2024
Hi Laurent,
On Fri, Sep 27, 2024 at 7:13 AM Laurent Pinchart
<laurent.pinchart at 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
More information about the libcamera-devel
mailing list