-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement SDL 0179 - Pixel density and Scale #1360
Changes from all commits
6d00081
addae68
a115a48
4e5b88c
6f46e1e
e4a8547
05ed747
f138bd5
fcf51e3
d499eff
31d9165
505ca7f
4cd321e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
@class SDLStateMachine; | ||
@class SDLStreamingMediaConfiguration; | ||
@class SDLTouchManager; | ||
@class SDLVideoStreamingCapability; | ||
|
||
@protocol SDLConnectionManagerType; | ||
@protocol SDLFocusableItemLocatorType; | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason you want to make |
||
|
||
@property (strong, nonatomic, readonly) SDLStateMachine *appStateMachine; | ||
@property (strong, nonatomic, readonly) SDLAppState *currentAppState; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This my idea of how to best prevent duplicate code for the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented here: |
||
|
||
@property (strong, nonatomic, readwrite) SDLStateMachine *videoStreamStateMachine; | ||
@property (strong, nonatomic, readwrite) SDLStateMachine *appStateMachine; | ||
|
||
|
@@ -393,7 +395,9 @@ - (void)didEnterStateVideoStreamReady { | |
NSAssert(self.videoFormat != nil, @"No video format is known, but it must be if we got a protocol start service response"); | ||
|
||
SDLLogD(@"Attempting to create video encoder"); | ||
self.videoEncoder = [[SDLH264VideoEncoder alloc] initWithProtocol:self.videoFormat.protocol dimensions:self.screenSize ssrc:self.ssrc properties:self.videoEncoderSettings delegate:self error:&error]; | ||
float scale = self.videoStreamingCapability.scale.floatValue; | ||
lnafarrateluxoft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
CGSize scaledScreenSize = CGSizeMake(self.screenSize.width / scale, self.screenSize.height / scale); | ||
self.videoEncoder = [[SDLH264VideoEncoder alloc] initWithProtocol:self.videoFormat.protocol dimensions:scaledScreenSize ssrc:self.ssrc properties:self.videoEncoderSettings delegate:self error:&error]; | ||
|
||
if (error || self.videoEncoder == nil) { | ||
SDLLogE(@"Could not create a video encoder: %@", error); | ||
|
@@ -710,6 +714,8 @@ - (void)sdl_requestVideoCapabilities:(SDLVideoCapabilityResponseHandler)response | |
} | ||
|
||
SDLVideoStreamingCapability *videoCapability = ((SDLGetSystemCapabilityResponse *)response).systemCapability.videoStreamingCapability; | ||
self.videoStreamingCapability = videoCapability; | ||
self.touchManager.videoStreamingCapability = videoCapability; | ||
SDLLogD(@"Video capabilities response received: %@", videoCapability); | ||
responseHandler(videoCapability); | ||
}]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
#import "SDLTouchCoord.h" | ||
#import "SDLTouchEvent.h" | ||
#import "SDLTouchManagerDelegate.h" | ||
#import "SDLVideoStreamingCapability.h" | ||
|
||
|
||
NS_ASSUME_NONNULL_BEGIN | ||
|
@@ -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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think |
||
|
||
SDLTouchType touchType = onTouchEvent.type; | ||
[onTouchEvent.event enumerateObjectsUsingBlock:^(SDLTouchEvent *touchEvent, NSUInteger idx, BOOL *stop) { | ||
|
@@ -212,6 +214,26 @@ - (void)sdl_onTouchEvent:(SDLRPCNotificationNotification *)notification { | |
}]; | ||
} | ||
|
||
/** | ||
* Modifies the existing coordinates of the SDLOnTouchEvent, based on the received 'scale' value. | ||
|
||
* This will match the coordinates to the scaled resolution of the video. | ||
|
||
* @param onTouchEvent A SDLOnTouchEvent with coordinates. | ||
*/ | ||
- (SDLOnTouchEvent *)sdl_applyScaleToEventCoordinates:(SDLOnTouchEvent *)onTouchEvent { | ||
float scale = self.videoStreamingCapability.scale.floatValue; | ||
if (scale > 1) { | ||
for (SDLTouchEvent *touchEvent in onTouchEvent.event) { | ||
for (SDLTouchCoord *coord in touchEvent.coord) { | ||
coord.x = @(coord.x.floatValue / scale); | ||
coord.y = @(coord.y.floatValue / scale); | ||
} | ||
} | ||
} | ||
return onTouchEvent; | ||
} | ||
|
||
lnafarrateluxoft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#pragma mark - Private | ||
/** | ||
* Handles a BEGIN touch event sent by Core | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,10 @@ | |
@implementation SDLVideoStreamingCapability | ||
|
||
- (instancetype)initWithPreferredResolution:(nullable SDLImageResolution *)preferredResolution maxBitrate:(int32_t)maxBitrate supportedFormats:(nullable NSArray<SDLVideoStreamingFormat *> *)supportedFormats hapticDataSupported:(BOOL)hapticDataSupported { | ||
return [self initWithPreferredResolution:preferredResolution maxBitrate:maxBitrate supportedFormats:supportedFormats hapticDataSupported:hapticDataSupported diagonalScreenSize:0 pixelPerInch:0 scale:SDLVideoStreamingCapability.sdl_DefaultScale.floatValue]; | ||
} | ||
|
||
- (instancetype)initWithPreferredResolution:(nullable SDLImageResolution *)preferredResolution maxBitrate:(int32_t)maxBitrate supportedFormats:(nullable NSArray<SDLVideoStreamingFormat *> *)supportedFormats hapticDataSupported:(BOOL)hapticDataSupported diagonalScreenSize:(float)diagonalScreenSize pixelPerInch:(float)pixelPerInch scale:(float)scale { | ||
self = [self init]; | ||
if (!self) { | ||
return self; | ||
|
@@ -27,6 +31,9 @@ - (instancetype)initWithPreferredResolution:(nullable SDLImageResolution *)prefe | |
self.preferredResolution = preferredResolution; | ||
self.supportedFormats = supportedFormats; | ||
self.hapticSpatialDataSupported = @(hapticDataSupported); | ||
self.diagonalScreenSize = @(diagonalScreenSize); | ||
self.pixelPerInch = @(pixelPerInch); | ||
self.scale = @(scale); | ||
|
||
return self; | ||
} | ||
|
@@ -63,6 +70,38 @@ - (void)setHapticSpatialDataSupported:(nullable NSNumber<SDLBool> *)hapticSpatia | |
return [self.store sdl_objectForName:SDLRPCParameterNameHapticSpatialDataSupported ofClass:NSNumber.class error:nil]; | ||
} | ||
|
||
- (void)setDiagonalScreenSize:(nullable NSNumber<SDLFloat> *)diagonalScreenSize { | ||
[self.store sdl_setObject:diagonalScreenSize forName:SDLRPCParameterNameDiagonalScreenSize]; | ||
} | ||
|
||
- (nullable NSNumber<SDLFloat> *)diagonalScreenSize { | ||
return [self.store sdl_objectForName:SDLRPCParameterNameDiagonalScreenSize ofClass:NSNumber.class error:nil]; | ||
} | ||
|
||
- (void)setPixelPerInch:(nullable NSNumber<SDLFloat> *)pixelPerInch { | ||
[self.store sdl_setObject:pixelPerInch forName:SDLRPCParameterNamePixelPerInch]; | ||
} | ||
|
||
- (nullable NSNumber<SDLFloat> *)pixelPerInch { | ||
return [self.store sdl_objectForName:SDLRPCParameterNamePixelPerInch ofClass:NSNumber.class error:nil]; | ||
} | ||
|
||
- (void)setScale:(nullable NSNumber<SDLFloat> *)scale { | ||
[self.store sdl_setObject:scale forName:SDLRPCParameterNameScale]; | ||
} | ||
|
||
- (nullable NSNumber<SDLFloat> *)scale { | ||
NSNumber<SDLFloat> *scale = [self.store sdl_objectForName:SDLRPCParameterNameScale ofClass:NSNumber.class error:nil]; | ||
if (scale != nil) { | ||
return scale; | ||
} | ||
return SDLVideoStreamingCapability.sdl_DefaultScale; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
|
||
+ (NSNumber<SDLFloat> *)sdl_DefaultScale { | ||
return @1.0; | ||
} | ||
|
||
@end | ||
|
||
NS_ASSUME_NONNULL_END |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,7 @@ | |
@interface SDLStreamingVideoLifecycleManager () | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests in this class are crashing due to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
@property (copy, nonatomic, readonly) NSString *appName; | ||
@property (copy, nonatomic, readonly) NSString *videoStreamBackgroundString; | ||
@property (strong, nonatomic, readwrite) SDLVideoStreamingCapability *videoStreamingCapability; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should match what is in the class being tested (missing the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented in (line 43): |
||
@end | ||
|
||
QuickSpecBegin(SDLStreamingVideoLifecycleManagerSpec) | ||
|
@@ -411,6 +412,14 @@ __block void (^sendNotificationForHMILevel)(SDLHMILevel hmiLevel, SDLVideoStream | |
}); | ||
|
||
describe(@"sending a video capabilities request", ^{ | ||
__block SDLImageResolution *resolution = [[SDLImageResolution alloc] initWithWidth:42 height:69]; | ||
__block int32_t maxBitrate = 12345; | ||
__block NSArray<SDLVideoStreamingFormat *> *testFormats = @[[[SDLVideoStreamingFormat alloc] initWithCodec:SDLVideoStreamingCodecH265 protocol:SDLVideoStreamingProtocolRTMP], [[SDLVideoStreamingFormat alloc] initWithCodec:SDLVideoStreamingCodecH264 protocol:SDLVideoStreamingProtocolRTP]]; | ||
__block BOOL testHapticsSupported = YES; | ||
__block float diagonalScreenSize = 22.0; | ||
__block float pixelPerInch = 96.0; | ||
__block float scale = 1.0; | ||
|
||
beforeEach(^{ | ||
[streamingLifecycleManager.videoStreamStateMachine setToState:SDLVideoStreamManagerStateStarting fromOldState:nil callEnterTransition:YES]; | ||
}); | ||
|
@@ -443,22 +452,13 @@ __block void (^sendNotificationForHMILevel)(SDLHMILevel hmiLevel, SDLVideoStream | |
}); | ||
|
||
context(@"and receiving a response", ^{ | ||
__block SDLImageResolution *resolution = nil; | ||
__block int32_t maxBitrate = 0; | ||
__block NSArray<SDLVideoStreamingFormat *> *testFormats = nil; | ||
__block BOOL testHapticsSupported = NO; | ||
|
||
beforeEach(^{ | ||
SDLGetSystemCapabilityResponse *response = [[SDLGetSystemCapabilityResponse alloc] init]; | ||
response.success = @YES; | ||
response.systemCapability = [[SDLSystemCapability alloc] init]; | ||
response.systemCapability.systemCapabilityType = SDLSystemCapabilityTypeVideoStreaming; | ||
|
||
resolution = [[SDLImageResolution alloc] initWithWidth:42 height:69]; | ||
maxBitrate = 12345; | ||
testFormats = @[[[SDLVideoStreamingFormat alloc] initWithCodec:SDLVideoStreamingCodecH265 protocol:SDLVideoStreamingProtocolRTMP], [[SDLVideoStreamingFormat alloc] initWithCodec:SDLVideoStreamingCodecH264 protocol:SDLVideoStreamingProtocolRTP]]; | ||
testHapticsSupported = YES; | ||
response.systemCapability.videoStreamingCapability = [[SDLVideoStreamingCapability alloc] initWithPreferredResolution:resolution maxBitrate:maxBitrate supportedFormats:testFormats hapticDataSupported:testHapticsSupported]; | ||
|
||
response.systemCapability.videoStreamingCapability = [[SDLVideoStreamingCapability alloc] initWithPreferredResolution:resolution maxBitrate:maxBitrate supportedFormats:testFormats hapticDataSupported:testHapticsSupported diagonalScreenSize:diagonalScreenSize pixelPerInch:pixelPerInch scale:scale]; | ||
[testConnectionManager respondToLastRequestWithResponse:response]; | ||
}); | ||
|
||
|
@@ -485,6 +485,12 @@ __block void (^sendNotificationForHMILevel)(SDLHMILevel hmiLevel, SDLVideoStream | |
expect(preferredResolution.resolutionHeight).to(equal(@69)); | ||
expect(preferredResolution.resolutionWidth).to(equal(@42)); | ||
}); | ||
|
||
it(@"should have correct video streaming capability values", ^{ | ||
expect(streamingLifecycleManager.videoStreamingCapability.diagonalScreenSize).to(equal(22.0)); | ||
expect(streamingLifecycleManager.videoStreamingCapability.pixelPerInch).to(equal(96.0)); | ||
expect(streamingLifecycleManager.videoStreamingCapability.scale).to(equal(1.0)); | ||
}); | ||
}); | ||
}); | ||
}); | ||
|
@@ -507,6 +513,8 @@ __block void (^sendNotificationForHMILevel)(SDLHMILevel hmiLevel, SDLVideoStream | |
testVideoHeader.frameData = SDLFrameInfoStartServiceACK; | ||
testVideoHeader.encrypted = YES; | ||
testVideoHeader.serviceType = SDLServiceTypeVideo; | ||
|
||
streamingLifecycleManager.videoStreamingCapability = [[SDLVideoStreamingCapability alloc] initWithPreferredResolution:resolution maxBitrate:maxBitrate supportedFormats:testFormats hapticDataSupported:testHapticsSupported diagonalScreenSize:diagonalScreenSize pixelPerInch:pixelPerInch scale:scale]; | ||
}); | ||
|
||
context(@"with data", ^{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misleading method name:
- (CGRect)sdl_scaledScreenSizeFrame {}
(See Apple Objective-C guideline)