[libcamera-devel] [PATCH 1/5] libcamera: media_device: Provide the default entity

[Kieran Bingham]

Hi Jacopo,

On 07/02/2019 22:06, Jacopo Mondi wrote:
> Hi Kieran,
>     ignorant question, is FL_DEFAULT an UVC specific thing?

I think it's a media controller thing.  I'm equally ignorant.
This code caused my pain of not being able to import the buffers when I
was working on it in the morning, and it made sense to refactor it out
so that it did not hide my issue, as 'getting the default entity' for
whatever purpose seems like a common thing to do at the MediaDevice level.



> 
> A quick grep on kernel sources returns:
> 
> $ git grep FL_DEFAULT drivers/media/
> drivers/media/platform/omap3isp/ispresizer.c:   res->video_out.video.entity.flags |= MEDIA_ENT_FL_DEFAULT;
> drivers/media/usb/uvc/uvc_entity.c:             entity->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT;
> 

Yes, I saw a similar (I guess exactingly so) grep when I did this :)


> So I guess not, as omap3 seems also omap3 uses that as well.
> 
> If that's not a UVC specificity, I welcome this change.
> 
> On Thu, Feb 07, 2019 at 09:21:15PM +0000, Kieran Bingham wrote:
>> Add a helper to identify the default entity from a media device.
>>
>> Utilise it in the two places which iterate the entities manually, to allocated
>> a V4L2Device from the default entity.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  src/libcamera/include/media_device.h  |  1 +
>>  src/libcamera/media_device.cpp        | 15 +++++++++++++++
>>  src/libcamera/pipeline/uvcvideo.cpp   | 10 ++++------
>>  test/v4l2_device/v4l2_device_test.cpp | 12 +++++-------
>>  4 files changed, 25 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
>> index 9f038093b2b2..361e8f4a4b86 100644
>> --- a/src/libcamera/include/media_device.h
>> +++ b/src/libcamera/include/media_device.h
>> @@ -42,6 +42,7 @@ public:
>>
>>  	const std::vector<MediaEntity *> &entities() const { return entities_; }
>>  	MediaEntity *getEntityByName(const std::string &name) const;
>> +	MediaEntity *defaultEntity() const;
>>
>>  	MediaLink *link(const std::string &sourceName, unsigned int sourceIdx,
>>  			const std::string &sinkName, unsigned int sinkIdx);
>> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
>> index 800ed330fe6d..4af90e1590a1 100644
>> --- a/src/libcamera/media_device.cpp
>> +++ b/src/libcamera/media_device.cpp
>> @@ -319,6 +319,21 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name) const
>>  	return nullptr;
>>  }
>>
>> +/**
>> + * \brief Return the default MediaEntity within a MediaDevice
>> + * \return The default entity if specified, or a nullptr otherwise
>> + */
>> +MediaEntity *MediaDevice::defaultEntity() const
>> +{
>> +	for (MediaEntity *entity : entities_) {
>> +		if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
>> +			return entity;
>> +		}
> 
> You could drop the inner braces { } here

Ack.


>> +	}
>> +
>> +	return nullptr;
>> +}
>> +
>>  /**
>>   * \brief Retrieve the MediaLink connecting two pads, identified by entity
>>   * names and pad indexes
>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
>> index fc31c52c0ecd..ed8228bb2fc6 100644
>> --- a/src/libcamera/pipeline/uvcvideo.cpp
>> +++ b/src/libcamera/pipeline/uvcvideo.cpp
>> @@ -146,13 +146,11 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>>
>>  	media_->acquire();
>>
>> -	for (MediaEntity *entity : media_->entities()) {
>> -		if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
>> -			video_ = new V4L2Device(entity);
>> -			break;
>> -		}
>> -	}
>> +	MediaEntity *entity = media_->defaultEntity();
>> +	if (!entity)
> 
> Looking at existing code if you don't find any DEFAULT entity, you
> return false as you do here, but also release the media device.
> 
> Should the same be done here?
> 

Ah yes, quite likely.


>> +		return false;
>>
>> +	video_ = new V4L2Device(entity);
>>  	if (!video_ || video_->open()) {
>>  		if (!video_)
>>  			LOG(UVC, Error) << "Could not find a default video device";
>> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp
>> index 18d014caf4c8..dd28cccada6b 100644
>> --- a/test/v4l2_device/v4l2_device_test.cpp
>> +++ b/test/v4l2_device/v4l2_device_test.cpp
>> @@ -46,15 +46,13 @@ int V4L2DeviceTest::init()
>>
>>  	media_->acquire();
>>
>> -	for (MediaEntity *entity : media_->entities()) {
>> -		if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
>> -			dev_ = new V4L2Device(entity);
>> -			break;
>> -		}
>> -	}
>> +	MediaEntity *entity = media_->defaultEntity();
>> +	if (!entity)
>> +		return TestFail;
>>
>> +	dev_ = new V4L2Device(entity);
>>  	if (!dev_)
>> -		return TestSkip;
>> +		return TestFail;
>>
>>  	return dev_->open();
>>  }
>> --
>> 2.19.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran