[PATCH 0/1] Add StreamRole into StreamConfiguration
Cheng-Hao Yang
chenghaoyang at chromium.org
Tue Sep 24 07:29:16 CEST 2024
On Tue, Sep 24, 2024 at 2:36 AM Nicolas Dufresne <nicolas at 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 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()`
> > 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
>
More information about the libcamera-devel
mailing list