Implement SDL 0179 - Pixel density and Scale #1360

lnafarrateluxoft · 2019-07-25T06:29:47Z

This PR is ready for review.

Risk

This PR makes minor API changes.

Testing Plan

Unit tests added for:

SDLStreamingVideoLifecycleManagerSpec
SDLVideoStreamingCapabilitySpec
SDLTouchManagerSpec

Functional testing can be done by editing the preferred resolution,
and scale in HMI and testing that the captured view has the proper size and touches are reported
correctly and with the correct coordinates while clicking on the HMI interface.
Also the Automatic Haptic Rect sent to HMI should have the propper size according to the scale value set.

Summary

The main change is related to changing the size of the view captured.
This is done by adding a new scale parameter in the video
streaming capability and by dividing the preferred resolution from the
HMI and the scale.

Also touch coordinates need some changes to match in the HMI with the new resolution, I had to divide the coordinates with the new scale value.

CLA

I have signed the CLA

…amingCapability'.

…nges in the view controller's size to the accepted video resolution divided by the scale.

…ith the scale.

… Scale' proposal.

…nsity_and_scale

lnafarrateluxoft · 2019-07-25T17:17:40Z

@joeljfischer please review.

theresalech · 2019-07-30T20:28:38Z

@kshala-ford can you please confirm if you have reviewed and approved this PR? Thank you!

ghost

I left a comment regarding the OnTouchEvent scaling. Other than that it looks good. I was missing code/tests regarding the hit tester (see https://github.com/smartdevicelink/sdl_ios/blob/master/SmartDeviceLink/SDLTouchManager.m#L89). This is using the focusable item tester here https://github.com/smartdevicelink/sdl_ios/blob/master/SmartDeviceLink/SDLFocusableItemLocator.h

Can you explain if the hit tester is covered by updating the touch event notifications?

SmartDeviceLink/SDLTouchManager.m

…el_density_and_scale

…deo streaming source match size.

NicoleYarroch

I left some comments on the code. When I run the all of the test cases all at once (Product > Test), 19 test case s fail. However, when I run the classes where the test cases fail individually, all the class test cases succeed. I tried to debug the test cases but I'm not sure exactly what is happening. I would recommend commenting out the changes you made and adding them back in one-by-one.

SmartDeviceLink/SDLVideoStreamingCapability.h

SmartDeviceLink/SDLCarWindow.m

SmartDeviceLink/SDLStreamingVideoLifecycleManager.m

SmartDeviceLink/SDLTouchManager.m

SmartDeviceLinkTests/RPCSpecs/StructSpecs/SDLVideoStreamingCapabilitySpec.m

NicoleYarroch · 2019-08-26T13:52:46Z

SmartDeviceLinkTests/UtilitiesSpecs/Touches/SDLTouchManagerSpec.m

@@ -23,6 +23,8 @@
 #import "SDLTouchManagerDelegate.h"
 #import "SDLTouchType.h"
 #import "SDLTouch.h"
+#import "SDLVideoStreamingCapability.h"


When I run all the test cases at once, I get 14 failing test cases in this class, but when I run just the test cases in this class they all pass. I'm baffled as to why this is happening. Maybe the new scale parameter is messing things up?

I did noticed that some of the tests are faulty, but at the end they pass.

The main problem there is that:

SDLTouchManager is recreated on every test.

SDLTouchManager subscribes for notifications in its -init* method (which is not the best solution by itself but it works)

SDLTouchManager never unsubscribes from notifications.

A few instances of SDLTouchManager receive notifications and handle them "properly"

Tests fail due to wrong event number

Hotfix in SDLTouchManagerSpec.m:

if (touchManager) { //FIXIT: SDLTouchManager must unsubscribe from notifications [[NSNotificationCenter defaultCenter] removeObserver:touchManager]; touchManager = nil; }

SmartDeviceLinkTests/DevAPISpecs/SDLStreamingVideoLifecycleManagerSpec.m

SmartDeviceLink/SDLTouchManager.h

…el_density_and_scale

lnafarrateluxoft · 2019-08-27T01:38:40Z

@NicoleYarroch Done. All your comments have been addressed in a new commit.

NicoleYarroch

Left some comments on the implementation and the tests.

NicoleYarroch · 2019-08-29T17:53:19Z

SmartDeviceLink/SDLVideoStreamingCapability.m

+    if (scale != nil) {
+        return scale;
+    }
+    return SDLVideoStreamingCapability.sdl_DefaultScale;


I see what you are trying to do here, but a custom getter should not be created in this class. The SDL convention is for this to return the value set by Core. In the future we might need to know if scale is null or if it has been set by Core and we wouldn't be able to get this information with the current setup. I added another comment to the SDLStreamingVideoLifecycleManager with an idea of how to best reuse the code.

NicoleYarroch · 2019-08-29T18:03:21Z

SmartDeviceLink/SDLStreamingVideoLifecycleManager.h

@@ -35,6 +36,7 @@ NS_ASSUME_NONNULL_BEGIN

 @property (strong, nonatomic, readonly) SDLStateMachine *videoStreamStateMachine;
 @property (strong, nonatomic, readonly) SDLVideoStreamManagerState *currentVideoStreamState;
+@property (nullable, strong, nonatomic, readonly) SDLVideoStreamingCapability *videoStreamingCapability;


Is there a reason you want to makevideoStreamingCapability public? Iit doesn't appear to be used by another class. If developers want to get the videoStreamingCapability they can use the SDLSystemCapabilityManager.

NicoleYarroch · 2019-08-29T18:12:08Z

SmartDeviceLink/SDLStreamingVideoLifecycleManager.m

@@ -58,6 +58,8 @@ @interface SDLStreamingVideoLifecycleManager() <SDLVideoEncoderDelegate>
 @property (assign, nonatomic, readonly, getter=isAppStateVideoStreamCapable) BOOL appStateVideoStreamCapable;
 @property (assign, nonatomic, readonly, getter=isHmiStateVideoStreamCapable) BOOL hmiStateVideoStreamCapable;

+@property (nullable, strong, nonatomic, readwrite) SDLVideoStreamingCapability *videoStreamingCapability;


This my idea of how to best prevent duplicate code for the scale value.

In the SDLStreamingVideoLifecycleManager create a custom sdl_scale getter to return videoStreamingCapability.scale or 1 if the scale value does not exists.

Add another init to SDLTouchManger that takes a scale value in addition to the hitTester. In the SDLStreamingVideoLifecycleManager class set sdl_scale for the scale value in the touch manager init. Then later, when we get the videoStreamingCapability from core, the actual scale value can be set in the touch manager class.

Do the same thing for SDLCarWindow. Add another init to SDLCarWindow that takes a scale value in addition to the streamManager and configuration. In the SDLStreamingVideoLifecycleManager class set sdl_scale for the scale value in the car window init. Then later, when we get the videoStreamingCapability from core, the actual scale value can be set in the car window class.

Implemented here:
yLeonid@91b30ce

NicoleYarroch · 2019-08-29T19:07:18Z

SmartDeviceLinkTests/RPCSpecs/StructSpecs/SDLVideoStreamingCapabilitySpec.m

@@ -42,7 +45,10 @@
        NSMutableDictionary* dict = [@{SDLRPCParameterNamePreferredResolution: resolution,
                                       SDLRPCParameterNameMaxBitrate: maxBitrate,
                                       SDLRPCParameterNameSupportedFormats: formatArray,
-                                       SDLRPCParameterNameHapticSpatialDataSupported: hapticDataSupported} mutableCopy];
+                                       SDLRPCParameterNameHapticSpatialDataSupported: hapticDataSupported,


Please add checks for the new values added to the dictionary to this test case:

expect(testStruct.diagonalScreenSize).to(equal(diagonalScreenSize)); expect(testStruct.pixelPerInch).to(equal(pixelPerInch)); expect(testStruct.scale).to(equal(scale));

NicoleYarroch · 2019-08-29T19:08:14Z

SmartDeviceLinkTests/RPCSpecs/StructSpecs/SDLVideoStreamingCapabilitySpec.m

@@ -60,6 +66,9 @@
        expect(testStruct.preferredResolution).to(beNil());
        expect(testStruct.maxBitrate).to(beNil());
        expect(testStruct.supportedFormats).to(beNil());
+        expect(testStruct.diagonalScreenSize).to(beNil());
+        expect(testStruct.pixelPerInch).to(beNil());
+        expect(testStruct.scale).to(equal(1));


This should be expect(testStruct.scale).to(beNil());

Fixed here:
lnafarrateluxoft@c8a9e5e

NicoleYarroch · 2019-08-29T19:10:21Z

SmartDeviceLinkTests/RPCSpecs/StructSpecs/SDLVideoStreamingCapabilitySpec.m

@@ -80,12 +92,15 @@

        NSArray<SDLVideoStreamingFormat *> *formatArray = @[format1, format2];

-        SDLVideoStreamingCapability *testStruct = [[SDLVideoStreamingCapability alloc] initWithPreferredResolution:resolution maxBitrate:maxBitrate supportedFormats:formatArray hapticDataSupported:hapticDataSupported];
+        SDLVideoStreamingCapability *testStruct = [[SDLVideoStreamingCapability alloc] initWithPreferredResolution:resolution maxBitrate:maxBitrate supportedFormats:formatArray hapticDataSupported:hapticDataSupported diagonalScreenSize:diagonalScreenSize pixelPerInch:pixelPerInch scale:scale];


Please add another test case so you have one test case for the new init and another test case for the deprecated init. The SDLMediaServiceDataSpec has an example of how to suppress warnings when testing a deprecated init.

Implemented here (lines 109-140)
lnafarrateluxoft@c8a9e5e

NicoleYarroch · 2019-08-29T20:07:07Z

SmartDeviceLinkTests/DevAPISpecs/SDLStreamingVideoLifecycleManagerSpec.m

@@ -40,6 +40,7 @@
 @interface SDLStreamingVideoLifecycleManager ()
 @property (copy, nonatomic, readonly) NSString *appName;
 @property (copy, nonatomic, readonly) NSString *videoStreamBackgroundString;
+@property (strong, nonatomic, readwrite) SDLVideoStreamingCapability *videoStreamingCapability;


This should match what is in the class being tested (missing the nullable)

@property (nullable, strong, nonatomic, readwrite) SDLVideoStreamingCapability *videoStreamingCapability;

Implemented in (line 43):
lnafarrateluxoft@c8a9e5e

NicoleYarroch · 2019-08-30T13:04:26Z

SmartDeviceLinkTests/DevAPISpecs/SDLStreamingVideoLifecycleManagerSpec.m

@@ -40,6 +40,7 @@
 @interface SDLStreamingVideoLifecycleManager ()


The tests in this class are crashing due to the videoStreamingCapability not yet being set in the SDLCarWindow class. In the sdl_getScaledScreenSizeFrame of the SDLCarWindow class it is dividing by a scale of 0. This would be solved by setting a default scale value in the init of SDLCarWindow

Implemented in
https://github.com/lnafarrateluxoft/sdl_ios/pull/1/files#diff-19e714f2ef0ceaa247b881f63bfd0c98

NicoleYarroch · 2019-08-30T17:45:52Z

SmartDeviceLink/SDLTouchManager.m

@@ -184,7 +185,8 @@ - (void)sdl_onTouchEvent:(SDLRPCNotificationNotification *)notification {
        return;
    }

-    SDLOnTouchEvent* onTouchEvent = (SDLOnTouchEvent*)notification.notification;
+    SDLOnTouchEvent *onTouchEvent = (SDLOnTouchEvent *)notification.notification;
+    onTouchEvent = [self sdl_applyScaleToEventCoordinates:onTouchEvent.copy];


I don't think copy is working as expected here which is why the TouchManager test cases are breaking when I run all the tests. Maybe copyWithZone: has a bug?

theresalech · 2019-09-05T13:00:24Z

@lnafarrateluxoft can you please let us know when you anticipate this PR being ready for re-review? Thank you!

yLeonid · 2019-09-11T10:45:48Z

SmartDeviceLink/SDLCarWindow.m

        SDLLogD(@"Video stream started, setting CarWindow frame to: %@", NSStringFromCGRect(self.rootViewController.view.bounds));
    });
 }

+- (CGRect)sdl_getScaledScreenSizeFrame {


Misleading method name:

We can use a property name without the prefix get
- (CGRect)sdl_scaledScreenSizeFrame {}

If we really want to use the prefix get then the method must look like this:

- (void)sdl_getScaledScreenSizeFrame:(CGRect * outRect) { if (outRect) { *outRect = <the rect value>; } }

(See Apple Objective-C guideline)

yLeonid · 2019-09-13T20:22:37Z

Travis tests had no chance to pass or even to get executed:
See Travis Job log:
error: SDK "iphonesimulator11.0" cannot be located.
BUT: exited with 0. <<== zero here means success but in fact tests were not even launched.

https://travis-ci.org/smartdevicelink/sdl_ios/builds/577331329?utm_source=github_status&utm_medium=notification

smartdevicelink#1360 (comment) smartdevicelink#1360 (comment)

…ecycleManager in init smartdevicelink#1360 (comment)

smartdevicelink#1360 (comment) smartdevicelink#1360 (comment)

smartdevicelink#1360 (comment) smartdevicelink#1360 (comment) update CarWindow and TouchManager to use scale from StreamingVideoLifecycleManager in init smartdevicelink#1360 (comment)

smartdevicelink#1360 (comment) smartdevicelink#1360 (comment) update private property of tested class in test to meet its declaration

dboltovskyi · 2019-09-16T14:12:34Z

@NicoleYarroch Please review updates related to your comments

NicoleYarroch · 2019-09-18T12:30:04Z

Closing this PR in favor of PR #1401

lnafarrateluxoft added 6 commits July 23, 2019 14:53

Adding new properties 'pixel', 'density' and 'scale' in 'SDLVideoStre…

6d00081

…amingCapability'.

Accessing SDLVideoStreamingCapability from SDLCarWindow, to apply cha…

addae68

…nges in the view controller's size to the accepted video resolution divided by the scale.

Calculating the correct coordinates by dividing the incoming coords w…

a115a48

…ith the scale.

Fixing tests for new changes in implementation for 'Pixel density and…

4e5b88c

… Scale' proposal.

Adding Unit Tests for Pixel density and Scale proposal.

6f46e1e

Merge remote-tracking branch 'upstream/develop' into feature/pixel_de…

e4a8547

…nsity_and_scale

lnafarrateluxoft force-pushed the feature/0179_pixel_density_and_scale branch from 428b300 to e4a8547 Compare July 25, 2019 06:30

Fixing unit tests.

05ed747

NicoleYarroch assigned lnafarrateluxoft Jul 25, 2019

NicoleYarroch added enhancement proposal Accepted SDL Evolution Proposal labels Jul 25, 2019

NicoleYarroch added this to the 6.4.0 milestone Jul 25, 2019

joeljfischer changed the title ~~[SDL 0179]Pixel density and Scale~~ Implement SDL 0179 - Pixel density and Scale Jul 29, 2019

ghost suggested changes Aug 2, 2019

View reviewed changes

SmartDeviceLink/SDLTouchManager.m Show resolved Hide resolved

lnafarrateluxoft added 2 commits August 5, 2019 13:44

Merge remote-tracking branch 'upstream/develop' into feature/0179_pix…

f138bd5

…el_density_and_scale

Addressing comment in PR to copy OnTouchEvent before modification.

fcf51e3

NicoleYarroch self-requested a review August 6, 2019 12:38

lnafarrateluxoft added 2 commits August 9, 2019 14:48

Merge remote-tracking branch 'upstream/develop' into feature/0179_pix…

d499eff

…el_density_and_scale

Fixing video frame dimensions, applying scale. Now video frame and vi…

31d9165

…deo streaming source match size.

NicoleYarroch suggested changes Aug 26, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into feature/0179_pix…

505ca7f

…el_density_and_scale

Addressing comments in PR.

4cd321e

lnafarrateluxoft force-pushed the feature/0179_pixel_density_and_scale branch from 699d4db to 4cd321e Compare August 27, 2019 12:52

NicoleYarroch suggested changes Aug 30, 2019

View reviewed changes

yLeonid reviewed Sep 11, 2019

View reviewed changes

yLeonid added a commit to yLeonid/sdl_ios that referenced this pull request Sep 13, 2019

update if statement using early return as proposed at:

7600d81

smartdevicelink#1360 (comment) smartdevicelink#1360 (comment)

yLeonid added a commit to yLeonid/sdl_ios that referenced this pull request Sep 13, 2019

update CarWindow and TouchManager to use scale from StreamingVideoLif…

91b30ce

…ecycleManager in init smartdevicelink#1360 (comment)

yLeonid added a commit to yLeonid/sdl_ios that referenced this pull request Sep 14, 2019

update SDLVideoStreamingCapabilitySpec tests as requested

046c0b9

smartdevicelink#1360 (comment) smartdevicelink#1360 (comment)

yLeonid added a commit to yLeonid/sdl_ios that referenced this pull request Sep 14, 2019

update SDLVideoStreamingCapabilitySpec tests as requested

c8a9e5e

smartdevicelink#1360 (comment) smartdevicelink#1360 (comment) update private property of tested class in test to meet its declaration

NicoleYarroch mentioned this pull request Sep 17, 2019

#0179 pixel density and scale; fix tests and apply comments; lnafarrateluxoft/sdl_ios#1

Closed

3 tasks

yLeonid mentioned this pull request Sep 17, 2019

Implement SDL 0179 - Pixel density and Scale #1401

Closed

NicoleYarroch closed this Sep 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SDL 0179 - Pixel density and Scale #1360

Implement SDL 0179 - Pixel density and Scale #1360

lnafarrateluxoft commented Jul 25, 2019 •

edited

Loading

lnafarrateluxoft commented Jul 25, 2019

theresalech commented Jul 30, 2019

ghost left a comment

NicoleYarroch left a comment

NicoleYarroch Aug 26, 2019

lnafarrateluxoft Aug 27, 2019

yLeonid Sep 13, 2019 •

edited

Loading

lnafarrateluxoft commented Aug 27, 2019

NicoleYarroch left a comment

NicoleYarroch Aug 29, 2019

NicoleYarroch Aug 29, 2019

NicoleYarroch Aug 29, 2019

yLeonid Sep 14, 2019

NicoleYarroch Aug 29, 2019

NicoleYarroch Aug 29, 2019

yLeonid Sep 14, 2019

NicoleYarroch Aug 29, 2019

yLeonid Sep 14, 2019

NicoleYarroch Aug 29, 2019

yLeonid Sep 14, 2019

NicoleYarroch Aug 30, 2019

yLeonid Sep 14, 2019

NicoleYarroch Aug 30, 2019

theresalech commented Sep 5, 2019

yLeonid Sep 11, 2019 •

edited

Loading

yLeonid commented Sep 13, 2019

dboltovskyi commented Sep 16, 2019

NicoleYarroch commented Sep 18, 2019

		@@ -40,6 +40,7 @@
		@interface SDLStreamingVideoLifecycleManager ()

Implement SDL 0179 - Pixel density and Scale #1360

Implement SDL 0179 - Pixel density and Scale #1360

Conversation

lnafarrateluxoft commented Jul 25, 2019 • edited Loading

Risk

Testing Plan

Summary

CLA

lnafarrateluxoft commented Jul 25, 2019

theresalech commented Jul 30, 2019

ghost left a comment

Choose a reason for hiding this comment

NicoleYarroch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yLeonid Sep 13, 2019 • edited Loading

Choose a reason for hiding this comment

lnafarrateluxoft commented Aug 27, 2019

NicoleYarroch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theresalech commented Sep 5, 2019

yLeonid Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

yLeonid commented Sep 13, 2019

dboltovskyi commented Sep 16, 2019

NicoleYarroch commented Sep 18, 2019

lnafarrateluxoft commented Jul 25, 2019 •

edited

Loading

yLeonid Sep 13, 2019 •

edited

Loading

yLeonid Sep 11, 2019 •

edited

Loading