Custom video encoder settings and dynamic screen sizes #406

asm09fsu · 2016-05-26T01:38:51Z

This PR is ready for review.

Risk

This PR makes minor API changes.

Testing Plan

For #399
We can test by starting a video session and validating that the encoder is in fact using the screen dimensions returned back from a register app interface response.

For #401
A valid testing plan would make sure that we cannot change the video encoder's settings while a video stream is connected, as well as validating that settings SDLStreamingMediaManager's new videoEncoderSettings property to nil results in using the default settings.

Summary

For #399
Added support for SDLStreamingMediaManager's initializer to use the screen dimensions returned from a successful RegisterAppInterface.

For #401
This PR allows a developer to specify custom video encoder settings if they believe they have found a better means of streaming with their current environment.

Changelog

Enhancements

Added ability for developer-specified VTCompressionSession properties.
Added support for providing SDLStreamingMediaManager with display capabilities returned from a successful RegisterAppInterface.

Bug Fixes

Fixed an issue relating to setting the return error when configuring the video encoder.

…tings.

joeljfischer · 2016-05-26T19:52:31Z

SmartDeviceLink-iOS/SmartDeviceLink/SDLStreamingMediaManager.m

-
-        return NO;
+
+    if (self.videoEncoderSettings == nil) {


If this is how it's being done, then the video encoder settings dict should be null_resettable, not nullable, and the api should be redesigned so that it's initialized on init, maybe an initWithEncoderSettings if possible. For testability, as many dependencies as possible should be injected at init time.

initWithEncoderSettings: would collide with other fixes coming down the pipeline (such as dynamic encoder dimensions). The thought here is that so long as the stream isn't open, we can modify the existing SDLStreamingMediaManager object, instead of having to specify it via the proxy (since the developer never calls the init). We can definitely add it in the init.

We need to decide how this init will work with #400 also making a change / addition to init.

…video_encoder_settings * origin/develop: (26 commits) add id attribute to section headers for linking Add link to smartdevicelink.com to README Update for v4.1.4 Fix not handling the TCP disconnect case Update README.md Update README.md Add documentation coverage badge to README Fix `.xctool-args` path to project Fix another codecov.yml issue Fix branch issue for codecov comments Basically just a bump to re-run tests Update for v4.1.3 Add clean switch to generate-documentation.sh Update gen documentation script Add Jazzy theme, scripts, initial documentation, and an updated gitignore Update codecov yaml file comment and status settings Fix mis-spaced README file causing header issues Fix test script Add carthage before_deploy generating of archive Fix a badge in the README ... # Conflicts: # SmartDeviceLink/SDLStreamingMediaManager.m

…e, because that is essentially what we are doing.

codecov-io · 2016-07-27T22:08:06Z

Current coverage is 69.04% (diff: 0.00%)

Merging #406 into develop will decrease coverage by 0.35%

@@            develop       #406   diff @@
==========================================
  Files           281        281          
  Lines          9376       9424    +48   
  Methods        2585       2592     +7   
  Messages          0          0          
  Branches        603        612     +9   
==========================================
  Hits           6507       6507          
- Misses         2581       2629    +48   
  Partials        288        288

Powered by Codecov. Last update 11b0b07...3638430

… when destructing the proxy.

…ster app interface for use in the video encoder.

asm09fsu · 2016-07-27T22:14:15Z

@joeljfischer Issue #400 has been added to this PR as well, with no modification of the API's init methods.

joeljfischer · 2016-07-28T17:36:29Z

SmartDeviceLink/SDLProxy.m

@@ -166,7 +169,11 @@ - (NSString *)proxyVersion {

 - (SDLStreamingMediaManager *)streamingMediaManager {
    if (_streamingMediaManager == nil) {
+        if (self.displayCapabilities == nil) {
+            @throw [NSException exceptionWithName:NSInvalidArgumentException reason:@"SDLStreamingMediaManager must be accessed only after a successful RegisterAppInterfaceResponse" userInfo:nil];
+        }
        _streamingMediaManager = [[SDLStreamingMediaManager alloc] initWithProtocol:self.protocol];


Can we deprecate initWithProtocol: and add initWithProtocol:displayCapabilities:? The displayCapabilities property on SMM will have to stay nullable for now, but that would clarify the actual dependencies.

…apabilities.

…init use.

joeljfischer · 2016-07-28T18:20:58Z

SmartDeviceLink/SDLStreamingMediaManager.m


    if (status != noErr) {
        // TODO: Log the error
-        if (*error != nil) {
+        if (*error == nil) {


These were correct originally. They're checking that the passed NSError address can be dereferenced, and later retrieved by the developer. See "Generating your own errors" at https://developer.apple.com/library/ios/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/ErrorHandling/ErrorHandling.html

joeljfischer · 2016-07-29T18:56:26Z

LGTM

joeljfischer · 2016-07-29T19:15:36Z

This was merged

asm09fsu added 2 commits May 25, 2016 18:28

Added support for allowing developers to set custom video encoder set…

aac227a

…tings.

Updated SDLStreamingManager's vide encoder to use optimal settings.

abfb28d

asm09fsu added enhancement proposal Accepted SDL Evolution Proposal labels May 26, 2016

asm09fsu added this to the 4.2 milestone May 26, 2016

joeljfischer reviewed May 26, 2016
View reviewed changes

asm09fsu changed the title ~~[WIP] Update video encoder settings & allow for custom settings~~ [WIP] Allow for custom video encoder settings May 26, 2016

asm09fsu changed the title ~~[WIP] Allow for custom video encoder settings~~ Allow for custom video encoder settings Jun 14, 2016

joeljfischer modified the milestones: 4.X, 4.2 Jul 27, 2016

asm09fsu added 3 commits July 27, 2016 14:46

Removed surrounding parenthesis on NSNumber-wrapped boolean.

f3eb424

Changed videoEncoderSettings to be null_resettable instead of nullabl…

4694398

…e, because that is essentially what we are doing.

asm09fsu changed the title ~~Allow for custom video encoder settings~~ Allow for custom video encoder settings including dynamic screen size support. Jul 27, 2016

asm09fsu added 2 commits July 27, 2016 15:12

Added nullable property to streaming media manager for proper cleanup…

74ae86f

… when destructing the proxy.

Added support for retrieving screen dimensions from a successful regi…

2e929ed

…ster app interface for use in the video encoder.

asm09fsu mentioned this pull request Jul 27, 2016

Video Encoder Support for Dynamic Screen Sizes #400

Closed

joeljfischer changed the title ~~Allow for custom video encoder settings including dynamic screen size support.~~ Custom video encoder settings and dynamic screen sizes Jul 28, 2016

joeljfischer reviewed Jul 28, 2016
View reviewed changes

asm09fsu added 4 commits July 28, 2016 10:46

Updated SDLStreamingMediaManager's init function to include display c…

1c82de4

…apabilities.

Consolidated inits with a base init that both the deprecated and new …

6bb971b

…init use.

Removed synthesize of videoEncoderSettings.

1ac0a6b

Removed external displayCapabilities property.

5f7b6c3

joeljfischer reviewed Jul 28, 2016
View reviewed changes

Fixed issue relating to error handling of settings up the encoder.

0f7226c

joeljfischer removed the proposal Accepted SDL Evolution Proposal label Jul 29, 2016

joeljfischer closed this Jul 29, 2016

This was referenced Jul 30, 2016

Video Encoder Should Not Use Constant Values #399

Closed

Allow for Developer to Set Video Encoder Settings #401

Closed

asm09fsu deleted the feature/issue_401_video_encoder_settings branch November 1, 2016 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom video encoder settings and dynamic screen sizes #406

Custom video encoder settings and dynamic screen sizes #406

asm09fsu commented May 26, 2016 •

edited

joeljfischer May 26, 2016

asm09fsu May 27, 2016 •

edited

joeljfischer Jul 27, 2016

codecov-io commented Jul 27, 2016 •

edited

asm09fsu commented Jul 27, 2016

joeljfischer Jul 28, 2016

joeljfischer Jul 28, 2016

joeljfischer commented Jul 29, 2016

joeljfischer commented Jul 29, 2016

Custom video encoder settings and dynamic screen sizes #406

Custom video encoder settings and dynamic screen sizes #406

Conversation

asm09fsu commented May 26, 2016 • edited

Risk

Testing Plan

Summary

Changelog

Enhancements

Bug Fixes

joeljfischer May 26, 2016

Choose a reason for hiding this comment

asm09fsu May 27, 2016 • edited

Choose a reason for hiding this comment

joeljfischer Jul 27, 2016

Choose a reason for hiding this comment

codecov-io commented Jul 27, 2016 • edited

Current coverage is 69.04% (diff: 0.00%)

asm09fsu commented Jul 27, 2016

joeljfischer Jul 28, 2016

Choose a reason for hiding this comment

joeljfischer Jul 28, 2016

Choose a reason for hiding this comment

joeljfischer commented Jul 29, 2016

joeljfischer commented Jul 29, 2016

asm09fsu commented May 26, 2016 •

edited

asm09fsu May 27, 2016 •

edited

codecov-io commented Jul 27, 2016 •

edited