Merge lp:~mterry/qtmir/ms-compressed-timestamps into lp:qtmir

[Michael Terry]

Hand Qt millisecond timestamps rather than nanosecond.

We do this by treating compressed timestamps as millisecond timestamps themselves. (We could alternately divide by 1000000 everywhere we use them, but since we use only use them when dealing with Qt, where we want to provide milliseconds, I figured this was less fragile.)

The new TimestampInMilliseconds test actually catches the error (Qt was seeing milliseconds). The changes to timestamp_test.cpp merely reflect the changes to compressed handling.

 * Are there any related MPs required for this MP to build/function as expected? Please list.
 No

 * Did you perform an exploratory manual test run of your code change and any related functionality?
 Only tests so far, I'm going to run this with my u8 branch that noticed this problem and see if it's fixed. Will keep you posted.

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
 NA

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:399
http://jenkins.qa.ubuntu.com/job/qtmir-ci/538/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/234
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/234
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/234/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/116
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/271/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/271/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/116/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/538/rebuild

review: Needs Fixing (continuous-integration)

[Michael Terry (mterry)]

OK, confirmed this fixes my issues with https://code.launchpad.net/~mterry/unity8/shutdown-dialog-on-resume/+merge/275240

Yay!

[Michael Terry (mterry)]

The wily CI failures seem unrelated.

[Gerry Boland (gerboland)]

+ std::chrono::nanoseconds result = (timestamp - startTime) / 1000000;
shouldn't the result be in std::chrono::milliseconds? chrono does implicit conversion between nano & milliseconds, should be able to use that instead of the explicit division.

Your editor does something odd to the file line endings, can you undo that?

I'll let Daniel or Nick review the principle

[Gerry Boland (gerboland)]

On testing, this problem was cause of https://bugs.launchpad.net/qtmir/+bug/1510571 and several other gesture failures! Nice catch!

[Michael Terry (mterry)]

> + std::chrono::nanoseconds result = (timestamp - startTime) / 1000000;
> shouldn't the result be in std::chrono::milliseconds? chrono does
> implicit conversion between nano & milliseconds, should be able to use
> that instead of the explicit division.

We could, but later in that function, we want to test for overflow. So we want to keep the delta calculation in a type that is the same size as the nanoseconds type (i.e. we don't want to lose any accuracy before checking if we're about to lose any accuracy).

So we either do what I did or (1) don't divide by 100000 here, (2) multiply std::chrono::nanoseconds(std::numeric_limits<T>::max()) by 1000000 in the later comparison, and finally (3) divide 'result' by 1000000 right before returning.

But dividing by 100000 from the start seemed easier. I added a comment to explain that I was doing the division just for convenience of the overflow check later.

But I'm fine with doing the 1.2.3. steps instead if you think that they are more obvious.

[Michael Terry (mterry)]

Alright, I'll close this down in favor of dednick's branch: https://code.launchpad.net/~unity-team/qtmir/1510571.ms-timestamp-compression/+merge/276112

Unmerged revisions

399. By Michael Terry on 2015-10-28

Treat compressed timestamps as milliseconds

=== modified file 'src/common/timestamp_impl.h'
--- src/common/timestamp_impl.h	2015-09-16 17:04:52 +0000
+++ src/common/timestamp_impl.h	2015-10-28 20:02:22 +0000
@@ -12,7 +12,7 @@
 T compressTimestamp(std::chrono::nanoseconds timestamp)
 {
     std::chrono::nanoseconds startTime = getStartTime(timestamp);
-    std::chrono::nanoseconds result = timestamp - startTime;
+    std::chrono::nanoseconds result = (timestamp - startTime) / 1000000; // ms in nanosecond type to compare overflow
 
     if (std::numeric_limits<std::chrono::nanoseconds::rep>::max() > std::numeric_limits<T>::max() &&
         result > std::chrono::nanoseconds(std::numeric_limits<T>::max())) {
@@ -27,8 +27,8 @@
 template<typename T>
 std::chrono::nanoseconds uncompressTimestamp(T timestamp)
 {
-    auto tsNS = std::chrono::nanoseconds(timestamp);
+    auto tsNS = std::chrono::milliseconds(timestamp);
     return getStartTime(tsNS, false) + std::chrono::nanoseconds(tsNS);
 }
 
-}
\ No newline at end of file
+}

=== modified file 'tests/mirserver/QtEventFeeder/qteventfeeder_test.cpp'
--- tests/mirserver/QtEventFeeder/qteventfeeder_test.cpp	2015-10-21 11:46:33 +0000
+++ tests/mirserver/QtEventFeeder/qteventfeeder_test.cpp	2015-10-28 20:02:22 +0000
@@ -265,3 +265,18 @@
 
     ASSERT_TRUE(Mock::VerifyAndClearExpectations(mockWindowSystem));
 }
+
+TEST_F(QtEventFeederTest, TimestampInMilliseconds)
+{
+    setIrrelevantMockWindowSystemExpectations();
+    EXPECT_CALL(*mockWindowSystem, handleTouchEvent(_,0,_,_,_)).Times(1);
+    auto ev1 = mev::make_event(MirInputDeviceId(), std::chrono::milliseconds(123), 0 /* mac */, 0);
+    qtEventFeeder->dispatch(*ev1);
+    ASSERT_TRUE(Mock::VerifyAndClearExpectations(mockWindowSystem));
+
+    setIrrelevantMockWindowSystemExpectations();
+    EXPECT_CALL(*mockWindowSystem, handleTouchEvent(_,2,_,_,_)).Times(1);
+    auto ev2 = mev::make_event(MirInputDeviceId(), std::chrono::milliseconds(125), 0 /* mac */, 0);
+    qtEventFeeder->dispatch(*ev2);
+    ASSERT_TRUE(Mock::VerifyAndClearExpectations(mockWindowSystem));
+}

=== modified file 'tests/modules/General/timestamp_test.cpp'
--- tests/modules/General/timestamp_test.cpp	2015-09-16 16:19:00 +0000
+++ tests/modules/General/timestamp_test.cpp	2015-10-28 20:02:22 +0000
@@ -41,7 +41,7 @@
 
         ulong compressedTimestamp = qtmir::compressTimestamp<ulong>(timestamp);
 
-        EXPECT_EQ(addToTimestamp.count(), compressedTimestamp);
+        EXPECT_EQ(addToTimestamp.count(), compressedTimestamp * 1000000);
         EXPECT_EQ(qtmir::uncompressTimestamp<ulong>(compressedTimestamp), timestamp);
 
         addToTimestamp += std::chrono::milliseconds(1);
@@ -63,10 +63,10 @@
     quint32 compressedTimestamp = qtmir::compressTimestamp<quint32>(timestamp);
 
     // Add the quint32 limit +1 to get an overflow when we compress the timestamp
-    timestamp += std::chrono::nanoseconds(std::numeric_limits<quint32>::max()) + std::chrono::nanoseconds(1);
+    timestamp += std::chrono::milliseconds(std::numeric_limits<quint32>::max()) + std::chrono::milliseconds(1);
     compressedTimestamp = qtmir::compressTimestamp<quint32>(timestamp);
 
     EXPECT_EQ(0, compressedTimestamp);
     // ensure the uncompression will yields the original timestamp
     EXPECT_EQ(qtmir::uncompressTimestamp<quint32>(compressedTimestamp), timestamp);
-}
\ No newline at end of file
+}