Implement SDL 0048 RTP Video Encoder #703

shoamano83 · 2017-08-23T06:51:34Z

Fixes #619

This PR is ready for review.

Risk

This PR makes minor API changes.

This PR adds new public methods in SDLStreamingMediaManager, SDLStreamingMediaLifecycleManager and SDLVideoEncoder. Also, it updates the default flow of video streaming in sdl_videoEncoderOutputCallback().

I carefully implemented the changes not to break compatibility and I believe the risk is low, but I hope them to be double checked.

Testing Plan

Unit test cases are added for newly added classes.

Summary

Implements SDL 0048

This PR adds RTP packetizer implementation which is necessary for SDL-0048.

It also adds following changes which are necessary for the RTP format to work:

Existing implementation of SDLVideoEncoder is split into two parts. One is to extract NAL units from sample buffer, and the other is to add start codes in front of every NAL unit (which is in SDLH264ByteStreamPacketizer class)
New public method -sendVideoData:pts: is added so that SDL app can provide PTS (presentation timestamp) along with the video frame

Changelog

Breaking Changes

None

Enhancements

Update SDLVideoEncoder to extract list of NAL units from encoded data
Add SDLH264ByteStreamPacketizer to append start codes in front of the NAL units
Add SDLRTPH264Packetizer to create RTP packets from the NAL units
Add public method -sendVideoData:pts:

Bug Fixes

None

Tasks Remaining:

None

CLA

I have signed the CLA

This packetizer sends H.264 video stream using RTP frames defined by RFC 6184 and RFC 4571.

Also, latest PTS value is kept in SDLStreamingMediaLifecycleManager so that video image with outdated PTS will be rejected.

shoamano83 · 2017-08-23T06:53:14Z

@joeljfischer Please kindly review.

joeljfischer · 2017-08-23T13:36:32Z

@shoamano83 Because this adds a method to SDLStreamingMediaManager, which is a public class, this proposal makes minor API changes. I've updated your PR post accordingly.

joeljfischer

Hi @shoamano83, mostly asking for small changes. One big current question is that this appears to be adding code for RTP, but it's not currently enabled. I'm assuming that it won't be until the control payload / video capabilities PR for 5.0?

I've tested against Ford's Sync 3 TDK and everything is still working there, so I just still need to test against your recommended GStreamer settings.

joeljfischer · 2017-08-25T13:33:09Z

SmartDeviceLink/SDLH264Packetizer.h

+//  SmartDeviceLink-iOS
+//
+//  Created by Sho Amano on 4/11/17.
+//  Copyright © 2017 Xevo Inc. All rights reserved.


Hi @joeljfischer, I’m aware of the copyright headers in other files, but it is a request from our legal department to add our company name in the copyright header for newly created and updated files. I hope this is OK for you.

joeljfischer · 2017-08-25T13:34:02Z

SmartDeviceLink/SDLH264Packetizer.h

+
+#import <Foundation/Foundation.h>
+
+@protocol SDLH264Packetizer


All protocols, extensions, and classes need to be wrapped in NS_ASSUME_NONNULL_BEGIN and NS_ASSUME_NONNULL_END and nullable tags when the developer is allowed to pass nil.

Thanks, fixed by afecd28.

joeljfischer · 2017-08-25T13:34:41Z

SmartDeviceLink/SDLH264Packetizer.h

+ *       All NAL units that belongs to a frame should be included in
+ *       nalUnits array.
+ */
+- (NSArray *)createPackets:(NSArray *)nalUnits pts:(double)pts;


All container classes need generics to specify what is to be passed in or out.

I would prefer to see the pts:pts part expanded out of the acronym to presentationTimestamp:(double)presentationTimestamp

Fixed by 107382f.

joeljfischer · 2017-08-25T13:40:22Z

SmartDeviceLink/SDLH264Packetizer.h

+ *
+ * @return List of NSData. Each NSData holds a packet.
+ *
+ * @note This method cannot be called more than once with same pts value.


This should probably be @warning instead of @note

Updated by 107382f.

joeljfischer · 2017-08-25T13:40:53Z

SmartDeviceLink/SDLRTPH264Packetizer.h

+ *
+ * @note This must be between 0 and 127. Default value is 96.
+ */
+@property (assign, nonatomic) UInt8 payloadType;


Is there a link or reference to explain what this means?

I've put some documentation with commit d585057. Hope this will make clearer.

joeljfischer · 2017-08-25T14:33:42Z

SmartDeviceLink/SDLRTPH264Packetizer.m

+const UInt8 DEFAULT_PAYLOAD_TYPE = 96;
+const NSUInteger FU_INDICATOR_LEN = 1;
+const NSUInteger FU_HEADER_LEN = 1;
+const UInt8 TYPE_FU_A = 0x1C;


Unsure what "FU" or "A" are.

"A" seems to be just a name of a version (there are "FU-A" and "FU-B" in the RFC). I've updated the constant's name in d585057, hope this is OK.

joeljfischer · 2017-08-25T14:41:27Z

SmartDeviceLink/SDLStreamingMediaLifecycleManager.h

+ *
+ *  @return Whether or not the data was successfully encoded and sent.
+ */
+- (BOOL)sendVideoData:(CVImageBufferRef)imageBuffer pts:(CMTime)pts;


I would prefer to see the pts:pts part expanded out of the acronym to presentationTimestamp:(CMTime)presentationTimestamp

Sure, updated with 33b5d07.

joeljfischer · 2017-08-25T14:48:58Z

SmartDeviceLink/SDLStreamingMediaLifecycleManager.m

@@ -517,8 +538,14 @@ - (void)sdl_sendBackgroundFrames {
        return;
    }

+    const CMTime interval = CMTimeMake(1, 30);


Is this still okay even if they're not doing 30fps?

I think it should work, since the delta of timestamp doesn’t need to be constant all the time.

I just want to make doubly sure, because devs could try to send fps anywhere from half to double this number that this is going to work for them.

joeljfischer · 2017-08-25T14:49:19Z

SmartDeviceLink/SDLStreamingMediaManager.h

+ *
+ *  @return Whether or not the data was successfully encoded and sent.
+ */
+- (BOOL)sendVideoData:(CVImageBufferRef)imageBuffer pts:(CMTime)pts;


I would prefer to see the pts:pts part expanded out of the acronym to presentationTimestamp:(CMTime)presentationTimestamp

Updated with 33b5d07.

joeljfischer · 2017-08-25T14:52:35Z

SmartDeviceLink/SDLVideoEncoder.m

+
+- (BOOL)encodeFrame:(CVImageBufferRef)imageBuffer pts:(CMTime)pts {
+    if (!CMTIME_IS_VALID(pts)) {
+        pts = CMTimeMake(self.currentFrameNumber, 30);


Will this still work even if they're not using 30fps? Shouldn't we be pulling this from the videoEncoderSettings?

Actually this is from current implementation, I kept it as it is. (Please refer to: https://github.com/smartdevicelink/sdl_ios/blob/release/5.0.0/SmartDeviceLink/SDLVideoEncoder.m#L122)

My thought is that app should provide timestamp along with video frames, especially when RTP format is used.
If it cannot provide, then SDL Proxy may create timestamps by capturing current time when -sendVideoData: is called. I was hesitated to add this implementation because it may change behavior even when we use byte-stream format.

I’m aware of kVTCompressionPropertyKey_ExpectedFrameRate key in the configuration. But I thought we cannot guarantee that all navi apps add it in the configuration dictionary.

Can you add this as an issue? I agree with keeping it as is for now.

OK, I created issue #717.

Reflecting code review comments.

- Update "note" comment to "warning" - Add generics to NSArray containers - rename "pts" arg to "presentationTimestamp" - remove redundant method declarations - remove redundant protocol conformations

Reflecting review comments.

shoamano83 · 2017-08-28T12:29:34Z

Hi @joeljfischer, thank you for your detailed review.

One big current question is that this appears to be adding code for RTP, but it's not currently enabled. I'm assuming that it won't be until the control payload / video capabilities PR for 5.0?

That’s right, it is not enabled yet, and it should be enabled when the format is chosen through Constructed Payload + Video Capabilities mechanism.

- Add documentations in private functions - Rename private functions and args - Rename “pts” acronym - Remove “FU” acronym in comments

- Update initializer style - Add brackets between a boolean expression - Use dot syntax - Remove an extra line

Reflecting code review comment.

Reflecting review comments.

joeljfischer · 2017-08-29T20:07:58Z

Hi @shoamano83, I've tried to test the RTP streaming by changing SDLVideoEncoder.m L113 to instantiate an SDLRTPH264Packetizer and am running into issues. We couldn't get the socket to work (got connection refused, is there additional setup for using that mode?) so we attempted VideoStreamConsumer = pipe, but got invalid RTP payload errors.

We tested against a develop version of Core running on Ubuntu 16.04 with gStreamer v1.8.2.

rtp_test.txt

shoamano83 · 2017-08-29T21:57:47Z

Hi @joeljfischer, I've never tried pipe transport in my environment. Let me check and get back to you.

shoamano83 · 2017-08-30T09:51:54Z

I verified that socket as well as pipe works on my environment. Actually it's quite tricky to test with sdl_hmi and GStreamer. Here is details of what I do.

Environment:

Ubuntu 16.04 (amd64) on VirtualBox
SDL Core: develop branch, commit a2be84f2bbe469cbcaa345e812936f5e55a92291 with modification to log4cxx.properties
sdl_hmi: master branch, commit 18460356c02e985df598b94f5a6bdbba1c451fff + modification
GStreamer: 1.8.3

Mobile: iPhone 5s (iOS 8.4.1)
iOS Proxy: latest feature/rtp_packetizer_5.0.0 + modification
app: our proprietary app using OpenGL

Modifcation to sdl_hmi:

diff --git a/app/model/sdl/Abstract/Model.js b/app/model/sdl/Abstract/Model.js
index 194d30e..cb8f35e 100644
--- a/app/model/sdl/Abstract/Model.js
+++ b/app/model/sdl/Abstract/Model.js
@@ -510,11 +510,11 @@ SDL.SDLModel = Em.Object.extend({
       if (SDL.SDLController.getApplicationModel(appID).navigationStream !==
         null) {
 
-        SDL.SDLModel.data.naviVideo = document.getElementById('html5Player');
-        SDL.SDLModel.data.naviVideo.src = SDL.SDLController.getApplicationModel(
-          appID
-        ).navigationStream;
-        SDL.SDLModel.data.naviVideo.play();
+        //SDL.SDLModel.data.naviVideo = document.getElementById('html5Player');
+        //SDL.SDLModel.data.naviVideo.src = SDL.SDLController.getApplicationModel(
+        //  appID
+        //).navigationStream;
+        //SDL.SDLModel.data.naviVideo.play();
       }
     },

Modification to iOS proxy:

Change:

    _packetizer = [[SDLH264ByteStreamPacketizer alloc] init];

to:

    _packetizer = [[SDLRTPH264Packetizer alloc] init];

Modification to SDL Core:

Open log4cxx.properties and change this line:

log4j.rootLogger=ALL, SmartDeviceLinkCoreLogFile

to:

log4j.rootLogger=INFO, SmartDeviceLinkCoreLogFile

This prevents SDL Core from generating a huge amount of logs during streaming. In my environment they often exhaust RAM and Linux will stop working.

Steps:

Launch SDL Core using $ ./start.sh
Start sdl_hmi: $ google-chrome index.html
On HMI, click "OK" then click "i" icon
Start mobile app and connect through TCP transport
Click on the app icon on HMI. (Note: sometimes the app automatically resumes and this is not necessary)
HMI should ask you "Would you like to start video streaming?" so click "OK"
IMMEDIATELY start GStreamer pipeline on a terminal:

$ gst-launch-1.0 souphttpsrc location=http://127.0.0.1:5050 ! "application/x-rtp-stream" ! rtpstreamdepay ! "application/x-rtp,media=(string)video,clock-rate=90000,encoding-name=(string)H264" ! rtph264depay ! "video/x-h264, stream-format=(string)avc, alignment=(string)au" ! avdec_h264 ! videoconvert ! ximagesink sync=false

I noticed that on my Ubuntu 16.04 autovideosink doesn't work for some reasons. I've updated the pipeline to use videoconvert and ximagesink which should work on most Linux environments.

For pipe streamer, update smartDeviceLink.ini and also update the gstreamer pipeline as you tried:

$ gst-launch-1.0 filesrc location=/path/to/sdl_core_build_dir/bin/storage/video_stream_pipe ! "application/x-rtp-stream" ! rtpstreamdepay ! "application/x-rtp,media=(string)video,clock-rate=90000,encoding-name=(string)H264" ! rtph264depay ! "video/x-h264, stream-format=(string)avc, alignment=(string)au" ! avdec_h264 ! videoconvert ! ximagesink sync=false

Note, the whole sequence sometimes fails (possibly because of timing issue between HMI and GStreamer?) so I try it several times.

shoamano83 · 2017-08-30T09:56:36Z

JFYI, attaching a video of the whole sequence.
MAH06813.zip

autovideosink doesn't work in some environments, replacing it with ximagesink.

shoamano83 added 3 commits August 23, 2017 15:11

Extract H.264 ByteStream packetizer from SDLVideoEncoder

07f9f71

Add RTP packetizer

0e5ec40

This packetizer sends H.264 video stream using RTP frames defined by RFC 6184 and RFC 4571.

Add public method -sendVideoData:pts:

e2f4b79

Also, latest PTS value is kept in SDLStreamingMediaLifecycleManager so that video image with outdated PTS will be rejected.

joeljfischer changed the title ~~Feature/rtp packetizer 5.0.0~~ RTP Video Encoder Aug 23, 2017

joeljfischer changed the title ~~RTP Video Encoder~~ Implement SDL 0048 RTP Video Encoder Aug 23, 2017

joeljfischer self-requested a review August 23, 2017 13:38

joeljfischer assigned shoamano83 Aug 23, 2017

joeljfischer added enhancement proposal Accepted SDL Evolution Proposal labels Aug 23, 2017

joeljfischer added this to the 5.0.0 milestone Aug 23, 2017

joeljfischer suggested changes Aug 25, 2017

View reviewed changes

shoamano83 added 3 commits August 28, 2017 20:12

Add NS_ASSUME_NONNULL_BEGIN / NS_ASSUME_NONNULL_END

afecd28

Reflecting code review comments.

Reflect code review comments

107382f

- Update "note" comment to "warning" - Add generics to NSArray containers - rename "pts" arg to "presentationTimestamp" - remove redundant method declarations - remove redundant protocol conformations

Update documentation and constants in SDLRTPH264Packetizer

d585057

Reflecting review comments.

shoamano83 added 7 commits August 29, 2017 17:25

Update "pts" args in test cases

36449f8

Reflecting review comments

fe8772c

- Add documentations in private functions - Rename private functions and args - Rename “pts” acronym - Remove “FU” acronym in comments

Reflecting review comments

b934ad9

- Update initializer style - Add brackets between a boolean expression - Use dot syntax - Remove an extra line

Add dedicated methods for creating RTP frames

2b3e382

Reflecting code review comment.

fix: properly convert NSMutableArray to NSArray

97e0226

Reflecting code review comment.

Update "pts" acronym to "presentationTimestamp"

33b5d07

Reflecting review comments.

fix style issues in test cases

018cde1

Reflecting review comments.

Update comment about GStreamer pipeline

7f13c72

autovideosink doesn't work in some environments, replacing it with ximagesink.

joeljfischer approved these changes Aug 31, 2017

View reviewed changes

joeljfischer merged commit 39b0105 into smartdevicelink:release/5.0.0 Aug 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SDL 0048 RTP Video Encoder #703

Implement SDL 0048 RTP Video Encoder #703

shoamano83 commented Aug 23, 2017 •

edited by joeljfischer

Loading

shoamano83 commented Aug 23, 2017

joeljfischer commented Aug 23, 2017

joeljfischer left a comment

joeljfischer Aug 25, 2017

shoamano83 Aug 29, 2017

joeljfischer Aug 29, 2017

joeljfischer Aug 25, 2017

shoamano83 Aug 28, 2017

joeljfischer Aug 25, 2017

joeljfischer Aug 25, 2017

shoamano83 Aug 28, 2017

joeljfischer Aug 25, 2017

shoamano83 Aug 28, 2017

joeljfischer Aug 25, 2017

shoamano83 Aug 28, 2017

joeljfischer Aug 25, 2017

shoamano83 Aug 28, 2017

joeljfischer Aug 25, 2017

shoamano83 Aug 29, 2017

joeljfischer Aug 25, 2017

shoamano83 Aug 29, 2017

joeljfischer Aug 29, 2017

joeljfischer Aug 25, 2017

shoamano83 Aug 29, 2017

joeljfischer Aug 25, 2017

shoamano83 Aug 29, 2017

joeljfischer Aug 29, 2017

shoamano83 Aug 30, 2017

shoamano83 commented Aug 28, 2017

joeljfischer commented Aug 29, 2017 •

edited

Loading

shoamano83 commented Aug 29, 2017

shoamano83 commented Aug 30, 2017

shoamano83 commented Aug 30, 2017


		#import <Foundation/Foundation.h>

		@protocol SDLH264Packetizer

Implement SDL 0048 RTP Video Encoder #703

Implement SDL 0048 RTP Video Encoder #703

Conversation

shoamano83 commented Aug 23, 2017 • edited by joeljfischer Loading

Risk

Testing Plan

Summary

Changelog

Breaking Changes

Enhancements

Bug Fixes

Tasks Remaining:

CLA

shoamano83 commented Aug 23, 2017

joeljfischer commented Aug 23, 2017

joeljfischer 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

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

shoamano83 commented Aug 28, 2017

joeljfischer commented Aug 29, 2017 • edited Loading

shoamano83 commented Aug 29, 2017

shoamano83 commented Aug 30, 2017

Environment:

Modifcation to sdl_hmi:

Modification to iOS proxy:

Modification to SDL Core:

Steps:

shoamano83 commented Aug 30, 2017

shoamano83 commented Aug 23, 2017 •

edited by joeljfischer

Loading

joeljfischer commented Aug 29, 2017 •

edited

Loading