Skip to content
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

RealSense Device over Network #5999

Merged
merged 155 commits into from
Mar 26, 2020
Merged

Conversation

apuzhevi
Copy link
Contributor

@apuzhevi apuzhevi commented Mar 9, 2020

Do not merge, work in progress

This PR is adding Linux server tool (intended to run on Raspberry Pi 4 or similar) and an extension library to librealsense that allows connecting to this server.
Based on live555 for streaming and lz4 and libjpeg-turbo for compression

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
add_subdirectory(src/ethernet)
endif()

add_subdirectory(src/compression)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider moving under src/ethernet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to make COMPRESSION factory available LRS wide?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending

include/librealsense2-net/rs_net.h Outdated Show resolved Hide resolved

~ip_device();

ip_sensor *remote_sensors[NUM_OF_SENSORS];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use std::vector<unique_ptr<ip_sensor>> instead, it's better

Copy link
Contributor

@nhershko nhershko Mar 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make_shared is not fully supported at c++11. as well at my setup.
@dorodnic , are you suggesting to use shared_ptr or to leave it as is for now?
my source:
[(https://stackoverflow.com/questions/24609271/errormake-unique-is-not-a-member-of-std)]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after viewing the ip_sensors usages I will use shared_ptr. WIP.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not blocking issue.
New task for post release was created: https://rsjira.intel.com/browse/CAMOE-279

#include <ipDeviceCommon/RsCommon.h>

#define POOL_SIZE 100 //TODO:: to define the right value
#define MAX_FRAME_SIZE 1280 * 720 * 3 //TODO:: to define the right value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend against using this memory pool implementation - max frame size can be larger in near future, and it's significant amount of memory to hold just in case.
Also, librealsense already has memory pool.
I recommend to use new / delete for now and eventually port to librealsense mem-pool in the future
not blocker

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

W'll handle it after the first version

}
case RS2_FORMAT_RGBA8:
{
bpp = 3;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bpp = 4?

{
// W/A - thease options fail on get range
// TODO Michal: check why...
if (opt == RS2_OPTION_LASER_POWER || opt == RS2_OPTION_EMITTER_ENABLED)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check, should work

scheduler = BasicTaskScheduler::createNew();
env = BasicUsageEnvironment::createNew(*scheduler);

rtspServer = RsRTSPServer::createNew(*env, 8554);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add command line option to configure the port

Copy link
Collaborator

@ev-mp ev-mp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General comments:
LRS uses C++ Camel notation for naming
Tab=4 spaces

include/librealsense2/hpp/rs_internal.hpp Outdated Show resolved Hide resolved
src/compression/JpegCompression.cpp Outdated Show resolved Hide resolved
src/compression/JpegCompression.cpp Outdated Show resolved Hide resolved
src/compression/JpegCompression.cpp Outdated Show resolved Hide resolved
src/compression/JpegCompression.cpp Outdated Show resolved Hide resolved
if (resultCode != 0)
{
env << "Failed to get a SDP description: " << resultString << "\n";
delete[] resultString;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleting memory allocated elsewhere should be reserved for extreme cases. Additionally the same member is used above in rsRtspClient->m_lastReturnValue.msg = resultString; which may lead to dangling ptr.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do it due to Live555 implementation, in order to fix it we will need to do risky changes in the Live555.
We wont have dangling ptr because m_lastReturnValue.msg is string and not char*


// Create a media session object from this SDP description:
scs.m_session = RsMediaSession::createNew(env, sdpDescription);
delete[] sdpDescription; // because we don't need it anymore
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same

src/ethernet/RsRtspClient.cpp Outdated Show resolved Hide resolved

// TODO: update width and height in subsession?
long long int uniqueKey = getStreamProfileUniqueKey(videoStream);
// TODO Michal: should the map key be long long?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

// TODO: when to delete p?
}
//TODO:remove this loop - once will be at API function?
} while (0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls check whether this loop is needed and remove if not

nhershko and others added 28 commits March 25, 2020 09:30
Installation fixes. Zlib removed.
@dorodnic dorodnic merged commit 9426f24 into IntelRealSense:development Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants