Mir

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

add-screencast-size-and-region-support
Merge into development-branch

Proposed by Alberto Aguirre on 2014-03-05

Status:

Merged

Approved by:

Alexandros Frantzis on 2014-03-11

Approved revision:

no longer in the source branch.

Merged at revision:

1465

Proposed branch:

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

Merge into:

lp://qastaging/mir

Diff against target:

1211 lines (+335/-247)

19 files modified

include/server/mir/frontend/screencast.h (+3/-1)
include/shared/mir_toolkit/client_types.h (+23/-3)
include/test/mir_test_doubles/mock_screencast.h (+4/-2)
include/test/mir_test_doubles/null_screencast.h (+3/-1)
src/client/CMakeLists.txt (+1/-0)
src/client/mir_screencast.cpp (+20/-23)
src/client/mir_screencast.h (+4/-2)
src/client/mir_screencast_api.cpp (+9/-19)
src/server/compositor/compositing_screencast.cpp (+14/-37)
src/server/compositor/compositing_screencast.h (+6/-4)
src/server/compositor/screencast_display_buffer.cpp (+3/-2)
src/server/frontend/session_mediator.cpp (+9/-4)
src/shared/protobuf/mir_protobuf.proto (+9/-1)
src/utils/screencast.cpp (+14/-12)
tests/acceptance-tests/test_client_screencast.cpp (+72/-39)
tests/integration-tests/client/test_screencast.cpp (+5/-1)
tests/unit-tests/client/test_mir_screencast.cpp (+91/-79)
tests/unit-tests/compositor/test_compositing_screencast.cpp (+28/-17)
tests/unit-tests/compositor/test_screencast_display_buffer.cpp (+17/-0)

To merge this branch:

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

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Alan Griffiths			Approve on 2014-03-11
Daniel van Vugt			Approve on 2014-03-11
PS Jenkins bot (community)	continuous-integration		Approve on 2014-03-10
Alexandros Frantzis (community)		2014-03-05	Approve on 2014-03-07
Review via email: mp+209380@code.qastaging.launchpad.net

Commit message

Add wiring to support screencasting a region of the screen and capturing at
a user supplied size.

This is useful to capture at a lower resolution to improve screencasting
performance or to capture only a region of interest.

Description of the change

A different MP will add options to the mirscreencast utility so users can take advantage of this functionality.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-05:

What you call "MirScreenRegion" is always a rectangle. Often "Region" refers to a more free-form region than that. So I suggest renaming "MirScreenRegion" to "MirRectangle".

Revision history for this message

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

> What you call "MirScreenRegion" is always a rectangle. Often "Region" refers
> to a more free-form region than that. So I suggest renaming "MirScreenRegion"
> to "MirRectangle".

Agree, I've changed it.

Revision history for this message

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

Since we are now moving to capturing arbitrary portions of the output(s), I propose the following interface:

typedef struct MirRectangle
{
    int32_t left; /* Note signed int */
    int32_t top; /* Note signed int */
    uint32_t width;
    uint32_t height;
} MirRectangle;

typedef struct MirScreencastParameters
{
    MirRectangle region;
    uint32_t width;
    uint32_t height;
    MirPixelFormat pixel_format;
} MirScreencastParameters;

That is, remove the OutputId parameter (will need changes in all relevant interfaces across the stack) and allow the user to capture an arbitrary rectangle in the *virtual coordinate space*. The mirscreencast utility can still keep the '-o' (output-id) parameter for convenience; we will just need to get the position/size of that output (we have that info in MirDisplayOutput) and use them as the region when creating the screencast.

This change will also allow capturing the contents of multiple outputs in the same screencast session.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-05:

There's no need to constrain ints while we already have the requirement that the client and server are on the same machine. As such, we should change:
int32_t --> int
uint32_t --> unsigned

On some 64-bit platforms this will yield better performance too.

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

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/997/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

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

^---those failures seem unrelated to the MP.

Revision history for this message

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

Functionally looks good (tested on desktop).

140 + if (output_size.width.as_int() == 0 ||
141 + output_size.height.as_int() == 0 ||
142 + region.size.width.as_int() == 0 ||
143 + region.size.height.as_int() == 0 ||
144 + pixel_format == mir_pixel_format_invalid) {
145 + BOOST_THROW_EXCEPTION(std::runtime_error("Invalid parameters"));
146 + }

static const geom::Size null_size;
if (output_size == null_size || region.size == null_size ||
pixel_format == mir_pixel_format_invalid)
{
...
}

391 + MirPixelFormat pixel_format = static_cast<MirPixelFormat>(parameters->pixel_format());

Could be const.

410 + required int32 region_top = 3;
411 + required int32 region_left = 4;
412 + required uint32 region_width = 5;
413 + required uint32 region_height = 6;

Perhaps we should have a Rectangle protobuf message?

review: Needs Fixing

Revision history for this message

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

> 140 + if (output_size.width.as_int() == 0 ||
> 141 + output_size.height.as_int() == 0 ||
> 142 + region.size.width.as_int() == 0 ||
> 143 + region.size.height.as_int() == 0 ||
> 144 + pixel_format == mir_pixel_format_invalid) {
> 145 + BOOST_THROW_EXCEPTION(std::runtime_error("Invalid parameters"));
> 146 + }
>
> static const geom::Size null_size;
> if (output_size == null_size || region.size == null_size ||
> pixel_format == mir_pixel_format_invalid)
> {
> ...
> }
>

I want to test for cases where only one parameter was set to 0.

> 391 + MirPixelFormat pixel_format =
> static_cast<MirPixelFormat>(parameters->pixel_format());
>
> Could be const.

Ack.

> 410 + required int32 region_top = 3;
> 411 + required int32 region_left = 4;
> 412 + required uint32 region_width = 5;
> 413 + required uint32 region_height = 6;
>
> Perhaps we should have a Rectangle protobuf message?

Ack.

Revision history for this message

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

> I want to test for cases where only one parameter was set to 0.

Right! In that case:

144 + pixel_format == mir_pixel_format_invalid) {
145 + BOOST_THROW_EXCEPTION(std::runtime_error("Invalid parameters"));
146 + }

Not our preferred brace style.

Revision history for this message

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

> > I want to test for cases where only one parameter was set to 0.
>
> Right! In that case:
>
> 144 + pixel_format == mir_pixel_format_invalid) {
> 145 + BOOST_THROW_EXCEPTION(std::runtime_error("Invalid parameters"));
> 146 + }
>
> Not our preferred brace style.

Fixed.

Revision history for this message

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

FAILED: Continuous integration, rev:1457
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1008/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1158
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1156
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/740/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/740/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/745/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/741/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1008/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/1010/rebuild

review: Approve (continuous-integration)

Revision history for this message

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

Looks good.

review: Approve

Revision history for this message

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

40 + MirRectangle region;
41 + unsigned int width;
42 + unsigned int height;

It looks odd to have a rectangle and width and height - at the least there should be some documentation (if not clearer names).

review: Needs Fixing

Revision history for this message

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

> 40 + MirRectangle region;
> 41 + unsigned int width;
> 42 + unsigned int height;
>
> It looks odd to have a rectangle and width and height - at the least there
> should be some documentation (if not clearer names).

I added documentation. The main purpose of the seperate width/height from the region is to downscale the screencast independently of selecting a screen region to capture.

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-11:

Minor concerns, but nothing blocking;

(1) Pointless cast of MirPixelFormat to MirPixelFormat:
405 + MirPixelFormat const pixel_format = static_cast<MirPixelFormat>(parameters->pixel_format());

(2) You're not meant to ever change "required" stuff but I don't see a better answer...
427 - required uint32 output_id = 1;
428 + required Rectangle region = 1;

(3) The new protocol doesn't need or use output_id so why print it?
500 + std::cout << "Starting screencast for output id " << output_id << std::endl;

review: Approve

Revision history for this message

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

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2014-03-11:

(1) Oops. That cast is not pointless at all. Ignore (1) :)

Revision history for this message

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

> Minor concerns, but nothing blocking;

> (3) The new protocol doesn't need or use output_id so why print it?
> 500 + std::cout << "Starting screencast for output id " << output_id <<
> std::endl;

This is removed on the other MP - adding-extra-options-to-mirscreencast