Mir

Merge lp://qastaging/~albaguirre/mir/add-extra-options-to-mirscreencast into lp://qastaging/mir

add-extra-options-to-mirscreencast
Merge into development-branch

Proposed by Alberto Aguirre on 2014-03-06

Status:

Merged

Approved by:

Alberto Aguirre on 2014-03-11

Approved revision:

no longer in the source branch.

Merged at revision:

1466

Proposed branch:

lp://qastaging/~albaguirre/mir/add-extra-options-to-mirscreencast

Merge into:

lp://qastaging/mir

Prerequisite:

lp://qastaging/~albaguirre/mir/add-screencast-size-and-region-support

Diff against target:

344 lines (+157/-74)

1 file modified

src/utils/screencast.cpp (+157/-74)

To merge this branch:

bzr merge lp://qastaging/~albaguirre/mir/add-extra-options-to-mirscreencast

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Kevin DuBois (community)			Approve on 2014-03-10
PS Jenkins bot (community)	continuous-integration		Approve on 2014-03-10
Alan Griffiths			Approve on 2014-03-10
Alexandros Frantzis (community)		2014-03-06	Approve on 2014-03-07
Review via email: mp+209578@code.qastaging.launchpad.net

Commit message

Add extra options to mirscreencast utility.

Added the following options
-allow specifying the output filename
-allow directing output to stdout for piping content into other utilities
-allow customizing the screencast size
-allow specifying the screen region to capture

Description of the change

Add extra options to mirscreencast utility.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-06:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/998/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-03-06:

Great!

194 + ("display-id,d",
203 + ("screen-region,r",

Since it's not clear to the user which of the two takes precendence, I think we should fail if both are specified.

Some syntax nits:

88 + if (requested_region.size() == 4) {
89 + params.region.left = requested_region[0];
90 + params.region.top = requested_region[1];
91 + params.region.width = requested_region[2];
92 + params.region.height = requested_region[3];
93 + } else {
94 + params.region = get_screen_region_from(connection, output_id);
95 + }
96 +
97 + if (requested_size.size() == 2) {
98 + params.width = requested_size[0];
99 + params.height = requested_size[1];
100 + } else {
101 + params.width = params.region.width;
102 + params.height = params.region.height;
103 + }
104 +

Not our preferred brace syntax.

210 + try {
250 + if (vm.count("help")) {
289 + if (query_params_only) {
297 + if (output_filename.empty()) {

Not our preferred brace syntax.

277 + get_screencast_params(connection.get(), requested_size, screen_region, output_id);
291 + (egl_setup.pixel_read_format() == GL_BGRA_EXT ? "BGRA" : "RGBA") << std::endl;
293 + screencast_size.width << "x" << screencast_size.height << std::endl;
306 + use_std_out ? std::cout : std::move(std::ofstream(output_filename)));

Use 4 space indentation for line continuations (or align one char deeper than any '(' above).

259 + const char* socket_name = vm.count("mir-socket-file") ? socket_filename.c_str() : nullptr;

char const* is preferred (in our C++ code, at least).

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-06:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1007/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-07:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1009/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-03-07:

109 + params.pixel_format = mir_pixel_format_xbgr_8888;

We don't support all formats on all platforms (e.g. on mesa we don't currently support mir_pixel_format_xbgr_8888). We need to check the available formats returned bymir_connection_get_available_surface_formats() and choose a supported one of the type we need.

review: Needs Fixing

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-03-07:

For those trying out the branch: Note that you may see some strange frame rates and jumpiness on screen while taking a screencast. This is not caused by any screencasting changes, it's bug 1288570.

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-03-07:

> 109 + params.pixel_format = mir_pixel_format_xbgr_8888;
>
> We don't support all formats on all platforms (e.g. on mesa we don't currently
> support mir_pixel_format_xbgr_8888). We need to check the available formats
> returned bymir_connection_get_available_surface_formats() and choose a
> supported one of the type we need.

Oh..Fixed.

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-03-07:

Looks good.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-07:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1014/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-03-10:

A nit really, but:

332 + file_stream = std::move(std::unique_ptr<std::ostream>(new std::ofstream(output_filename)));

file_stream.reset(new std::ofstream(output_filename));

Although, I'd avoid the file_stream pointer and call do_screencast() with either cout or a local ofstream in the branches of the if.

review: Needs Fixing

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-03-10:

> A nit really, but:
> Although, I'd avoid the file_stream pointer and call do_screencast() with
> either cout or a local ofstream in the branches of the if.

Ok. I changed it to do that.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-03-10: