Update the TBScreenCapture #143
Conversation
|
Adding Cesar as he was the one to discuss this with me and I don't know who should review this: @Nitrillo |
Fix a typo
| @@ -110,7 +110,8 @@ - (void)initCapture { | |||
| dispatch_source_set_event_handler(_timer, ^{ | |||
| @autoreleasepool { | |||
| __block UIImage* screen = [_self screenshot]; | |||
| CGImageRef paddedScreen = [self resizeAndPadImage:screen]; | |||
| // CGImageRef paddedScreen = [self resizeAndPadImage:screen]; | |||
| CGImageRef paddedScreen = [screen CGImage]; | |||
There was a problem hiding this comment.
This is the critical line of the PR.
There was a problem hiding this comment.
I would remove the commented line of code.
| #define MAX_EDGE_SIZE_LIMIT 1280.0f | ||
| #define EDGE_DIMENSION_COMMON_FACTOR 16.0f | ||
| //// defines for image scaling | ||
| //// From https://bugs.chromium.org/p/webrtc/issues/detail?id=4643#c7 : |
There was a problem hiding this comment.
Lazy commenter. lol
|
I personally prefer not to leave huge blocks of commented code; scm is good for keeping the old stuff around if needed. As far as the function replaced, those changes were put in to deal with a number of corner cases with respect to keeping proper padding for the source image. A customer sent in an example that basically just used a webview in the root view controller and it was obvious that we needed to do some filtering in order to preserve all the pixels being tested. Have you had a chance to run these changes through a variety of different frame sizes? |
|
@wobbals I agree with the huge block. I am going to test on that. |
|
@wobbals I was testing on iPhone 6s, 5s, 4s, iPad pro, iPad air 2, iPad retina simulator. The previous distorted does go away. My Chrome browser version is: |
|
Please DO NOT MERGE until someone(probably) me to verify the one in Swift sample 😃 |
|
@chetanvangadiTokbox I was testing on iPhone 6s, 5s, 4s, iPad pro, iPad air 2, iPad retina simulator. The previous distorted does go away. My Chrome browser version is: Version 56.0.2924.76 (64-bit). The idea to remove this piece of code is to let another peer connection to know the exact size of the streaming screen. Previously, the video dimension was modified so it's off. |
|
The expected testing result will be: there is no distorted screen sharing on another peer connection. |
|
I tested on iPhone 6 plus, iphone 7 plus and iPad pro, I dont see any distorted image. |
| // UIGraphicsEndImageContext(); | ||
| // | ||
| // return [newImage CGImage]; | ||
| //} |
There was a problem hiding this comment.
Why is all the code above commented out? If it is no longer used remove it.
|
@Lucashuang0802 If there is code that is no longer used please remove it rather than commenting it out. The critical line change LGTM, but I think someone on the native team or @wobbals should probably sign off on it. It has been a while since I worked on iOS. |
| @@ -110,7 +110,7 @@ - (void)initCapture { | |||
| dispatch_source_set_event_handler(_timer, ^{ | |||
| @autoreleasepool { | |||
| __block UIImage* screen = [_self screenshot]; | |||
| CGImageRef paddedScreen = [self resizeAndPadImage:screen]; | |||
| CGImageRef paddedScreen = [screen CGImage]; | |||
There was a problem hiding this comment.
This is the critical line of the change.
|
@Lucashuang0802 We see distorted image with non standard resolution like 1282 x 720 with this merge, do not merge it |
…r-listener Add subscriber listener to the BasicVideoChat sample
We have been using this component in vertical solution for a couple iOS apps. We have a feature called screen sharing + annotation. We found it easier to achieve cross-platform annotation if we stream the intrinsic size of a UIView. Here is the list of modification: