[PATCH 0/1] Add StreamRole into StreamConfiguration
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Sep 23 17:10:15 CEST 2024
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 at 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 at ideasonboard.com>
> > > wrote:
> > >
> > > > Hi Harvey
> > > >
> > > > On Wed, Sep 18, 2024 at 04:33:27PM GMT, Cheng-Hao Yang wrote:
> > > > > Sorry Jacopo, my bad to miss the first message:
> > > > >
> > > > > On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi <
> > > > jacopo.mondi at ideasonboard.com>
> > > > > wrote:
> > > > >
> > > > > > Hi Harvey
> > > > > >
> > > > > > On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:
> > > > > > > Hi Jacopo,
> > > > > > >
> > > > > > > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi <
> > > > > > jacopo.mondi at ideasonboard.com>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi Harvey
> > > > > > > >
> > > > > > > > On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:
> > > > > > > > > Hi folks,
> > > > > > > > >
> > > > > > > > > Currently applications set resolutions, pixelFormat,
> > bufferCount,
> > > > > > etc,
> > > > > > > > > into StreamConfigurations, and Pipeline Handler decides which
> > > > streams
> > > > > > > > > they're assigned to. However, it doesn't allow application to
> > > > assign
> > > > > > > > > streams that cannot be distinguished by those arguments into
> > > > > > > > > VideoRecording or StillCapture (say YUV/NV12 format), which
> > is
> > > > > > needed in
> > > > > > > > > mtkisp7.
> > > > > > > >
> > > > > > > > Could you explain in a bit more detail why this "is needed"
> > and how
> > > > > > > > you plan to use StreamRole as part of the stream configuration
> > ?
> > > > > > > >
> > > > > >
> > > > > > Could you explain in a bit more detail why this "is needed" and how
> > > > > > you plan to use StreamRole as part of the stream configuration ?
> > > > > >
> > > > > >
> > > > > >
> > > > > In mtkisp7 pipeline handler, both preview (or video) and still
> > capture
> > > > > streams support the same format (NV12) and bufferCount, and the
> > > > > maximum resolutions are also the same. Therefore, when calling
> > > > > `validate()` and `configure()`, mtkisp7 pipeline handler doesn't know
> > > > > if applications/Android adapter wants the stream to go through the
> > > > > still captur pipeline or not.
> > > >
> > > > I still have an hard time understanding why validate() and configure()
> > > > needs to know the "role". Is this to assign "pipes" to Streams ?
> > >
> > >
> > > If I understand correctly that "pipes" means the pipelines of ISP/IPA
> > algos
> > > for preview/video/still capture respectively, yes, it's to assign the
> > > StreamConfiguration(s) to the desired pipe(s), which means to call
> > > `StreamConfiguration::setStream()`. According to the comments [1], it's
> > > supposed to be called in `PipelineHandler::configure()`, like
> > > SimplePipelineHandler [2]. However, there are also some pipeline handlers
> > > that set them in `CameraConfiguration::validate()` [3] [4].
> >
> > Indeed most of our documentation suggests that setStream() is meant to
> > be called during configure(). Considering that validate() is -always-
> > called before configure() by Camera, so where exactly you call it, I
> > don't think makes a big difference.
> >
> > We could also relax the documentation.
> >
>
> 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
More information about the libcamera-devel
mailing list