Add DPI information to video packets.

Add DPI information to video packets.

The intention is that capturers can add this information and the client can
make use of it to up-scale the desktop if it DPI is larger than that of the
host by an integer multiple. For example, a standard-DPI host viewed on a
retina-display Macbook Pro would no longer look tiny.

BUG=135089
TEST=EncoderVp8Test.TestDpiPropagation


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146715

[Jamie, 2012-07-12 01:34:10]

ptal

[Sergey Ulanov, 2012-07-12 01:39:36]

https://chromiumcodereview.appspot.com/10736046/diff/1/remoting/proto/video.p...
File remoting/proto/video.proto (right):

https://chromiumcodereview.appspot.com/10736046/diff/1/remoting/proto/video.p...
remoting/proto/video.proto:99: optional int32 dpi = 10;
would it be better to make this part of VideoPacketFormat. That's what we use to
send screen resolution.

[Wez, 2012-07-12 18:15:49]

https://chromiumcodereview.appspot.com/10736046/diff/1/remoting/proto/video.p...
File remoting/proto/video.proto (right):

https://chromiumcodereview.appspot.com/10736046/diff/1/remoting/proto/video.p...
remoting/proto/video.proto:99: optional int32 dpi = 10;
On 2012/07/12 01:39:36, sergeyu wrote:
> would it be better to make this part of VideoPacketFormat. That's what we use
to
> send screen resolution.

Agreed; this is part of the format.

https://chromiumcodereview.appspot.com/10736046/diff/1/remoting/proto/video.p...
remoting/proto/video.proto:99: optional int32 dpi = 10;
We should provide separate X & Y DPI values, so that we can cope with weird
pixel aspect in future.

[alexeypa (please no reviews), 2012-07-12 18:20:32]

On 2012/07/12 18:15:49, Wez wrote:
> We should provide separate X & Y DPI values, so that we can cope with weird
> pixel aspect in future.

A completely irrelevant drive-by but is there any use in passing mirroring
information or even a transformation matrix to generalize all possible
transformations?

[Jamie, 2012-07-12 21:22:55]

On 2012/07/12 01:39:36, sergeyu wrote:
>
https://chromiumcodereview.appspot.com/10736046/diff/1/remoting/proto/video.p...
> File remoting/proto/video.proto (right):
> 
>
https://chromiumcodereview.appspot.com/10736046/diff/1/remoting/proto/video.p...
> remoting/proto/video.proto:99: optional int32 dpi = 10;
> would it be better to make this part of VideoPacketFormat. That's what we use
to
> send screen resolution.

That's a good point. I'll change it to work that way.

[Jamie, 2012-07-12 21:24:51]

On 2012/07/12 18:20:32, alexeypa wrote:
> On 2012/07/12 18:15:49, Wez wrote:
> > We should provide separate X & Y DPI values, so that we can cope with weird
> > pixel aspect in future.
> 
> A completely irrelevant drive-by but is there any use in passing mirroring
> information or even a transformation matrix to generalize all possible
> transformations?

That feels like overkill for what we're trying to achieve here. If we did decide
to implement something like that, it wouldn't be in this CL I don't think.

[Jamie, 2012-07-13 19:59:44]

ptal

https://chromiumcodereview.appspot.com/10736046/diff/1/remoting/proto/video.p...
File remoting/proto/video.proto (right):

https://chromiumcodereview.appspot.com/10736046/diff/1/remoting/proto/video.p...
remoting/proto/video.proto:99: optional int32 dpi = 10;
On 2012/07/12 18:15:49, Wez wrote:
> We should provide separate X & Y DPI values, so that we can cope with weird
> pixel aspect in future.

Done.

https://chromiumcodereview.appspot.com/10736046/diff/1/remoting/proto/video.p...
remoting/proto/video.proto:99: optional int32 dpi = 10;
On 2012/07/12 01:39:36, sergeyu wrote:
> would it be better to make this part of VideoPacketFormat. That's what we use
to
> send screen resolution.

Done.

[Wez, 2012-07-13 21:49:01]

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/captu...
File remoting/base/capture_data.h (right):

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/captu...
remoting/base/capture_data.h:62: SkIPoint dpi() const { return dpi_; }
nit: Should this be SkISize?

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/encod...
File remoting/base/encoder_row_based.cc (right):

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/encod...
remoting/base/encoder_row_based.cc:128:
packet->mutable_format()->set_horizontal_dpi(dpi.x());
Only set of dpi() is non-zero.

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/encod...
File remoting/base/encoder_vp8.cc (right):

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/encod...
remoting/base/encoder_vp8.cc:297:
packet->mutable_format()->set_horizontal_dpi(dpi.x());
Only set these if dpi() is non-zero, otherwise the current Capturers will result
in zero DPI appearing on the wire.

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/proto/vide...
File remoting/proto/video.proto (right):

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/proto/vide...
remoting/proto/video.proto:37: // Horizontal and vertical DPI of the screen. If
either of these is zero or
nit: Rather than complicate calling code, mandate that they be unset or
non-zero?

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/proto/vide...
remoting/proto/video.proto:38: // unset, the corresponding DPI should be assumed
to be 96.
nit: It appears that Mac OS X defaults to 72dpi; my suggestion of 96 was based
on what I believe Windows assumes, so we should clarify where 96 comes from!

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/proto/vide...
remoting/proto/video.proto:39: optional int32 horizontal_dpi = 8;
nit: I think x_dpi and y_dpi would be sufficiently clear to the educated reader,
less to type, and would match the naming of the fields in SkIPoint that you've
used to carry DPI in CaptureData.

[Jamie, 2012-07-13 22:17:13]

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/captu...
File remoting/base/capture_data.h (right):

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/captu...
remoting/base/capture_data.h:62: SkIPoint dpi() const { return dpi_; }
On 2012/07/13 21:49:02, Wez wrote:
> nit: Should this be SkISize?

I had that at first, but the code ends up referring to the width and height of
the DPI, which doesn't seem right. SkIPoint isn't a perfect fit, but it seems
better than SkISize.

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/encod...
File remoting/base/encoder_row_based.cc (right):

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/encod...
remoting/base/encoder_row_based.cc:128:
packet->mutable_format()->set_horizontal_dpi(dpi.x());
On 2012/07/13 21:49:02, Wez wrote:
> Only set of dpi() is non-zero.

Done.

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/encod...
File remoting/base/encoder_vp8.cc (right):

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/encod...
remoting/base/encoder_vp8.cc:297:
packet->mutable_format()->set_horizontal_dpi(dpi.x());
On 2012/07/13 21:49:02, Wez wrote:
> Only set these if dpi() is non-zero, otherwise the current Capturers will
result
> in zero DPI appearing on the wire.

Done.

On a related note, since both encoders share code to copy fields from
CaptureData to VideoPacket, how would you feel about moving that code into a
CaptureData::CopyToVideoPacket method, probably as a separate CL?

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/proto/vide...
File remoting/proto/video.proto (right):

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/proto/vide...
remoting/proto/video.proto:37: // Horizontal and vertical DPI of the screen. If
either of these is zero or
On 2012/07/13 21:49:02, Wez wrote:
> nit: Rather than complicate calling code, mandate that they be unset or
> non-zero?

Does it complicate calling code? If the field is unset, won't the accessor
return zero? So if the caller has to cope with unset, it's actually simpler to
handle zero the same way (no need to call the has_ method).

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/proto/vide...
remoting/proto/video.proto:38: // unset, the corresponding DPI should be assumed
to be 96.
On 2012/07/13 21:49:02, Wez wrote:
> nit: It appears that Mac OS X defaults to 72dpi; my suggestion of 96 was based
> on what I believe Windows assumes, so we should clarify where 96 comes from!

Assuming most hosts are Windows, let's stick with 96 as the default, but I've
added a comment to clarify why.

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/proto/vide...
remoting/proto/video.proto:39: optional int32 horizontal_dpi = 8;
On 2012/07/13 21:49:02, Wez wrote:
> nit: I think x_dpi and y_dpi would be sufficiently clear to the educated
reader,
> less to type, and would match the naming of the fields in SkIPoint that you've
> used to carry DPI in CaptureData.

Done.

[Wez, 2012-07-14 00:55:11]

lgtm

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/captu...
File remoting/base/capture_data.h (right):

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/captu...
remoting/base/capture_data.h:62: SkIPoint dpi() const { return dpi_; }
On 2012/07/13 22:17:13, Jamie wrote:
> On 2012/07/13 21:49:02, Wez wrote:
> > nit: Should this be SkISize?
> 
> I had that at first, but the code ends up referring to the width and height of
> the DPI, which doesn't seem right. SkIPoint isn't a perfect fit, but it seems
> better than SkISize.

OK, SGTM

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/encod...
File remoting/base/encoder_vp8.cc (right):

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/base/encod...
remoting/base/encoder_vp8.cc:297:
packet->mutable_format()->set_horizontal_dpi(dpi.x());
On 2012/07/13 22:17:13, Jamie wrote:
> On 2012/07/13 21:49:02, Wez wrote:
> > Only set these if dpi() is non-zero, otherwise the current Capturers will
> result
> > in zero DPI appearing on the wire.
> 
> Done.
> 
> On a related note, since both encoders share code to copy fields from
> CaptureData to VideoPacket, how would you feel about moving that code into a
> CaptureData::CopyToVideoPacket method, probably as a separate CL?

In principle that's a layering violation since CaptureData is in base/ and
VideoPacket in protocol/ but clearly we're already violating that layering
here... Capturer, CaptureDate, Encoder & the implementations should move to
host/, really.

But yes, pulling out the initialise-from-CaptureData-fields code makes sense.

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/proto/vide...
File remoting/proto/video.proto (right):

https://chromiumcodereview.appspot.com/10736046/diff/4002/remoting/proto/vide...
remoting/proto/video.proto:37: // Horizontal and vertical DPI of the screen. If
either of these is zero or
On 2012/07/13 22:17:13, Jamie wrote:
> On 2012/07/13 21:49:02, Wez wrote:
> > nit: Rather than complicate calling code, mandate that they be unset or
> > non-zero?
> 
> Does it complicate calling code? If the field is unset, won't the accessor
> return zero? So if the caller has to cope with unset, it's actually simpler to
> handle zero the same way (no need to call the has_ method).

I think that's defined by the protobuf spec, yes, in which case it makes sense
to allow zero values, yes.

[commit-bot: I haz the power, 2012-07-14 01:01:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/10736046/20001

[commit-bot: I haz the power, 2012-07-14 02:35:17]

Change committed as 146715