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.

Sign up for GitHub

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

Update DisplayDriverServer and ClientDisplayDriver #1448

Open
wants to merge 5 commits into
base: RB-10.5
Choose a base branch
from
Open

Update DisplayDriverServer and ClientDisplayDriver #1448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9f85965
Update DisplayDriverServer and ClientDisplayDriver
Dec 3, 2024
6104274
Address John's comments
Dec 17, 2024
9c2a2e1
Add tests and expose merge driver map
Jan 9, 2025
f0a8f9f
Include image driver tests in merge tests
Jan 14, 2025
6b57e25
Make merge map a member variable
Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion include/IECoreImage/DisplayDriverServer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ class IECOREIMAGE_API DisplayDriverServer : public IECore::RunTimeTyped
using Port = int;
#endif

struct MergeDriverInfo
{
DisplayDriverPtr mergeDriver = nullptr;
int mergeCount = 0;
};

using MergeMap = std::map<int, MergeDriverInfo>;

using PortRange = std::pair<Port, Port>;

/// A port number of 0 causes a free port to be chosen
Expand All @@ -92,7 +100,6 @@ class IECOREIMAGE_API DisplayDriverServer : public IECore::RunTimeTyped
static const PortRange &registeredPortRange( const std::string &name );

private:

// Session class
// Takes care of one client connection.
class Session;
Expand All @@ -104,6 +111,8 @@ class IECOREIMAGE_API DisplayDriverServer : public IECore::RunTimeTyped
class PrivateData;
IE_CORE_DECLAREPTR( PrivateData );
PrivateDataPtr m_data;

MergeMap m_mergeMap;
};

IE_CORE_DECLAREPTR( DisplayDriverServer )
Expand Down
54 changes: 47 additions & 7 deletions src/IECoreImage/DisplayDriverServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@
#include "boost/bind/bind.hpp"

#include <thread>
#include <map>
#include <utility>
#include <optional>

#include <fcntl.h>
#ifndef _MSC_VER
Expand Down Expand Up @@ -95,7 +98,7 @@ class DisplayDriverServer::Session : public RefCounted
{
public:

Session( boost::asio::io_service& io_service );
Session( boost::asio::io_service& io_service, DisplayDriverServer::MergeMap& mergeMap );
~Session() override;

boost::asio::ip::tcp::socket& socket();
Expand All @@ -114,6 +117,8 @@ class DisplayDriverServer::Session : public RefCounted
DisplayDriverPtr m_displayDriver;
DisplayDriverServerHeader m_header;
CharVectorDataPtr m_buffer;
DisplayDriverServer::MergeMap& m_mergeMap;
std::optional<int> m_mergeId;
};

class DisplayDriverServer::PrivateData : public RefCounted
Expand Down Expand Up @@ -196,7 +201,7 @@ DisplayDriverServer::DisplayDriverServer( DisplayDriverServer::Port portNumber )
{
m_data = new DisplayDriverServer::PrivateData( portNumber );

DisplayDriverServer::SessionPtr newSession( new DisplayDriverServer::Session( m_data->m_service ) );
DisplayDriverServer::SessionPtr newSession( new DisplayDriverServer::Session( m_data->m_service, m_mergeMap ) );
m_data->m_acceptor.async_accept( newSession->socket(),
boost::bind( &DisplayDriverServer::handleAccept, this, newSession,
boost::asio::placeholders::error));
Expand Down Expand Up @@ -280,7 +285,7 @@ void DisplayDriverServer::handleAccept( DisplayDriverServer::SessionPtr session,
{
if (!error)
{
DisplayDriverServer::SessionPtr newSession( new DisplayDriverServer::Session( m_data->m_service ) );
DisplayDriverServer::SessionPtr newSession( new DisplayDriverServer::Session( m_data->m_service, m_mergeMap ) );
m_data->m_acceptor.async_accept( newSession->socket(),
boost::bind( &DisplayDriverServer::handleAccept, this, newSession,
boost::asio::placeholders::error));
Expand All @@ -292,8 +297,8 @@ void DisplayDriverServer::handleAccept( DisplayDriverServer::SessionPtr session,
* DisplayDriverServer::Session functions
*/

DisplayDriverServer::Session::Session( boost::asio::io_service& io_service ) :
m_socket( io_service ), m_displayDriver(nullptr), m_buffer( new CharVectorData( ) )
DisplayDriverServer::Session::Session( boost::asio::io_service& io_service, DisplayDriverServer::MergeMap& mergeMap ) :
m_socket( io_service ), m_displayDriver(nullptr), m_buffer( new CharVectorData( ) ), m_mergeMap( mergeMap )
{
}

Expand Down Expand Up @@ -363,7 +368,19 @@ void DisplayDriverServer::Session::handleReadHeader( const boost::system::error_
{
try
{
m_displayDriver->imageClose();
if ( !m_mergeId.has_value() )
{
m_displayDriver->imageClose();
}
else
{
auto &m = m_mergeMap.at(m_mergeId.value()); // Error out if not found
if ( --m.mergeCount <= 0 )
{
m_mergeMap.erase(m_mergeId.value());
m_displayDriver->imageClose();
}
}
}
catch ( std::exception &e )
{
Expand Down Expand Up @@ -424,8 +441,31 @@ void DisplayDriverServer::Session::handleReadOpenParameters( const boost::system
const StringData *displayType = parameters->member<StringData>( "remoteDisplayType", true /* throw if missing */ );

// create a displayDriver using the factory function.
m_displayDriver = DisplayDriver::create( displayType->readable(), displayWindow->readable(), dataWindow->readable(), channelNames->readable(), parameters );
if ( !parameters->member<IntData>( "displayDriverServer:mergeId", false ) )
{
m_displayDriver = DisplayDriver::create( displayType->readable(), displayWindow->readable(), dataWindow->readable(), channelNames->readable(), parameters );
}
else
{
m_mergeId = parameters->member<IntData>( "displayDriverServer:mergeId", false /* throw if missing */ )->readable();

// Check if merge ID in map, if not then create display driver and session count pair with merge ID.
auto &m = m_mergeMap[m_mergeId.value()];
if ( !m.mergeDriver )
{
const IntData *sessionClientsData = parameters->member<IntData>( "displayDriverServer:mergeClients", true /* throw if missing */ );
m.mergeDriver= DisplayDriver::create(
displayType->readable(),
displayWindow->readable(),
displayWindow->readable(), // For merge we want dataWindow = displayWindow
channelNames->readable(),
parameters
);
m.mergeCount = sessionClientsData->readable();
}
// Merge ID is now in map, so load the display driver.
m_displayDriver = m.mergeDriver;
}
scanLineOrder = m_displayDriver->scanLineOrderOnly();
acceptsRepeatedData = m_displayDriver->acceptsRepeatedData();
}
Expand Down
65 changes: 65 additions & 0 deletions test/IECoreImage/DisplayDriverServerTest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,20 @@

import unittest
import sys
import os
import imath

import IECore
import IECoreImage

class DisplayDriverServerTest( unittest.TestCase ) :

def __prepareBuf( self, buf, width, offset, red, green, blue ):
for i in range( 0, width ):
buf[3*i] = blue[i+offset]
buf[3*i+1] = green[i+offset]
buf[3*i+2] = red[i+offset]

def testPortNumber( self ) :

s1 = IECoreImage.DisplayDriverServer( 1559 )
Expand Down Expand Up @@ -118,6 +126,63 @@ def testPortRangeRegistry( self ) :
s2 = IECoreImage.DisplayDriverServer()
self.assertEqual( s2.portNumber(), 45021 )

def testMergeMap( self ) :
Copy link
Member

Choose a reason for hiding this comment

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

This approach to testing seems a little weak - there's nothing to show that the right data is sent successfully to the merged driver, or that the merged driver is closed at the right time. It also forces to expose the map publicly, which I think would be better left private. Can we beef this up by sending actual data and testing that it arrives in an ImageDisplayDriver appropriately?

Copy link
Author

@LinasBeres LinasBeres Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As much as I do agree that it is a little weak, I don't think it is the job of this test to make sure that the the image driver receives correct information since that is the job of the image writer test.

Here I'm only trying to make sure that the functionality of the merge map is adhered to. However, I do see your point that it's important to test that the image being sent over is closed correctly even if the code there hasn't changed.

I'll add similar tests as in testTransfer() in ImageDisplayDriverTest.py to make sure that the image is closed correctly and that data is being sent over.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is the job of this test to make sure that the the image driver receives correct information

We're adding functionality to route data from multiple client drivers into a single driver on the server. It seems pretty fundamental that we should test that the data actually arrives.

Copy link
Author

Choose a reason for hiding this comment

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

Also we have internal reasons at IE for exposing the map publicly

Copy link
Member

@johnhaddon johnhaddon Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What are the reasons? My reason for not wanting it to be public is that once it's in the API, it ties our hands in terms of future maintenance.

Copy link
Author

Choose a reason for hiding this comment

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

We wanted a way of checking which version of Cortex we're using. So querying if the function exists, but we could do something else.

server = IECoreImage.DisplayDriverServer( 45001 )

img = IECore.Reader.create( os.path.join( "test", "IECoreImage", "data", "tiff", "bluegreen_noise.400x300.tif" ) )()
self.assertEqual( img.keys(), [ 'B', 'G', 'R' ] )
red = img['R']
green = img['G']
blue = img['B']
width = img.dataWindow.max().x - img.dataWindow.min().x + 1

params = IECore.CompoundData()
params['displayHost'] = IECore.StringData('localhost')
params['displayPort'] = IECore.StringData( '45001' )
params['displayDriverServer:mergeId'] = IECore.IntData( 42 )
params['displayDriverServer:mergeClients'] = IECore.IntData( 2 )
params["remoteDisplayType"] = IECore.StringData( "ImageDisplayDriver" )
params["handle"] = IECore.StringData( "myHandle1" )

idd1 = IECoreImage.ClientDisplayDriver( img.displayWindow, img.dataWindow, list( img.channelNames() ), params )

params["handle"] = IECore.StringData( "myHandle2" )
idd2 = IECoreImage.ClientDisplayDriver( img.displayWindow, img.dataWindow, list( img.channelNames() ), params )

params['displayDriverServer:mergeId'] = IECore.IntData( 666 )
params['displayDriverServer:mergeClients'] = IECore.IntData( 1 )
params["handle"] = IECore.StringData( "myHandle3" )
idd3 = IECoreImage.ClientDisplayDriver( img.displayWindow, img.dataWindow, list( img.channelNames() ), params )

buf = IECore.FloatVectorData( width * 3 )
for i in range( 0, img.dataWindow.max().y - img.dataWindow.min().y + 1 ):
self.__prepareBuf( buf, width, i*width, red, green, blue )
idd1.imageData( imath.Box2i( imath.V2i( img.dataWindow.min().x, i + img.dataWindow.min().y ), imath.V2i( img.dataWindow.max().x, i + img.dataWindow.min().y) ), buf )
idd2.imageData( imath.Box2i( imath.V2i( img.dataWindow.min().x, i + img.dataWindow.min().y ), imath.V2i( img.dataWindow.max().x, i + img.dataWindow.min().y) ), buf )
idd3.imageData( imath.Box2i( imath.V2i( img.dataWindow.min().x, i + img.dataWindow.min().y ), imath.V2i( img.dataWindow.max().x, i + img.dataWindow.min().y) ), buf )

idd1.imageClose()
idd2.imageClose()
idd3.imageClose()

newImg = IECoreImage.ImageDisplayDriver.removeStoredImage( "myHandle1" )
newImg2 = IECoreImage.ImageDisplayDriver.removeStoredImage( "myHandle2" )
newImg3 = IECoreImage.ImageDisplayDriver.removeStoredImage( "myHandle3" )

# merge drivers share the same display driver - so second image should be none,
# as there is no image drivere associated with it.
self.assertIsNone( newImg2 )

# remove blindData for comparison
newImg.blindData().clear()
img.blindData().clear()
self.assertEqual( newImg, img )

newImg3.blindData().clear()
self.assertEqual( newImg3, img )

server = None

if __name__ == "__main__":
unittest.main()