Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move the libcamera_apps class implementation into its own cpp file. #47

Merged
merged 6 commits into from
Jul 6, 2021

Conversation

naushir
Copy link
Collaborator

@naushir naushir commented Jul 6, 2021

All tests still seem to pass, so that's good!

Instead of a template parameter, store an Options base class pointer in the
LibcameraApp class. This allows us to separate out the class implementation in
a separate cpp file in a subsequent change.
…ibcameraApp

This gives us quicker compilations for all the libcamera apps.
…aApp class.

This minor change allows the Options object to fully live within the class and
not have the application worry about lifetime management.
@naushir
Copy link
Collaborator Author

naushir commented Jul 6, 2021

@davidplowman something for you to ponder over.

@@ -301,7 +301,7 @@ def test_raw(dir):
retcode, time_taken = run_executable([executable, '-t', '2000', '-o', output_raw],
logfile)
check_retcode(retcode, "test_vid: raw test")
check_time(time_taken, 2, 5, "test_vid: raw test")
check_time(time_taken, 2, 8, "test_vid: raw test")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry, I always use an SSD. They're soooo fast!

@davidplowman
Copy link
Collaborator

Gosh, what a lot of tiny changes! I mean, that's always what it was going to take, but I think the separation and improved compile times are worth it.

One thought, have you tried the covariant return type trick? It could ensure that an app always makes the correct set of options (i.e. a user can't get it wrong) and it mostly takes care of the casting for you then.

Here's vaguely what I mean (sorry, long example):

#include <iostream>
#include <memory>

struct AppOptions
{
	AppOptions() : width(640), height (480) {}
	int width, height;
};

struct VideoOptions : public AppOptions
{
	VideoOptions() : AppOptions(), bitrate(10000000) {}
	unsigned int bitrate;
};

struct LibcameraApp
{
	LibcameraApp(AppOptions *opts = nullptr)
	{
		if (!opts)
			opts = new AppOptions;
		options = std::unique_ptr<AppOptions>(opts);
	}
	virtual ~LibcameraApp() {}
	virtual AppOptions const *Options() const { return options.get(); }
	std::unique_ptr<AppOptions> options;
};

struct LibcameraVid : public LibcameraApp
{
	LibcameraVid() : LibcameraApp(new VideoOptions) {}
	virtual VideoOptions const *Options() const override { return (VideoOptions *)options.get(); }
};

int main(int argc, char *argv[])
{
	LibcameraVid v;
	std::cout << v.Options()->bitrate << std::endl;
}

Notice how v.Options() gives you VideoOptions automatically, not AppOptions. I mean, I don't know if it's preferable, but what do you think?

@naushir
Copy link
Collaborator Author

naushir commented Jul 6, 2021

Yes, I could add something like that. Probably as a patch on-top of the existing ones, or perhaps as part of commit b9b5f24 that switches to using unique_ptrs.

@naushir
Copy link
Collaborator Author

naushir commented Jul 6, 2021

On second thoughts, I think there is going to be too much churn with this change, as I then have to convert all the LibcameraApp references passed all around the place into pointers so that we can correctly call the overloaded GetOptions().

@davidplowman, what do you think? Should we just bite the bullet and do it?

… class

Add a GetOption() method to retrieve the options struct from the application.
This can be overridden if an app derives from LibcameraApp and uses a derived
options struct.
…out.

My sdcard write speed would sometime cause these capture tests to timeout.
Previously other threads might be trying to queue new requests while
the camera is being stopped. This could produce warnings if
queueRequest fails. Now the two procedures are protected from one
another with the camera_stop_mutex_.

Signed-off-by: David Plowman <[email protected]>
@davidplowman davidplowman merged commit e077b31 into raspberrypi:main Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants