Commit 810be813 authored by Thibaut ROMAIN's avatar Thibaut ROMAIN
Browse files

REFAC: Use mutex in sensorTransformFactory like all other factories in OTB and...

REFAC: Use mutex in sensorTransformFactory like all other factories in OTB and call CleanFactories on GenericRsTranform Destructor
parent 221a249d
Pipeline #7304 failed with stages
in 94 minutes and 45 seconds
......@@ -23,6 +23,7 @@
#include "otbCompositeTransform.h"
#include "otbImageMetadata.h"
#include "otbSensorTransformFactory.h"
#include <string>
namespace otb
......@@ -178,6 +179,7 @@ protected:
GenericRSTransform();
~GenericRSTransform() override
{
SensorTransformFactory<TScalarType, NInputDimensions, NOutputDimensions>::CleanFactories();
}
void Modified() const override
......
......@@ -53,7 +53,7 @@ public:
/** Register one factory of this type */
static void RegisterOneFactory(void)
{
Pointer RPCForwardFactory = RPCForwardTransformFactory::New();
RPCForwardTransformFactory<TScalarType,NInputDimensions,NOutputDimensions>::Pointer RPCForwardFactory = RPCForwardTransformFactory::New();
itk::ObjectFactoryBase::RegisterFactory(RPCForwardFactory);
}
......
......@@ -30,7 +30,7 @@ namespace otb
template <class TScalarType, unsigned int NInputDimensions, unsigned int NOutputDimensions>
RPCForwardTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::RPCForwardTransformFactory()
{
this->RegisterOverride("otbSensorTransformBase", "otbRPCForwardTransform", "RPC Forward Transform", 1,
this->RegisterOverride("otbRPCTransformBase", "otbRPCForwardTransform", "RPC Forward Transform", 1,
itk::CreateObjectFunction<RPCForwardTransform<TScalarType,NInputDimensions,NOutputDimensions>>::New());
}
......
......@@ -52,7 +52,7 @@ public:
/** Register one factory of this type */
static void RegisterOneFactory(void)
{
Pointer RPCInverseFactory = RPCInverseTransformFactory::New();
RPCInverseTransformFactory<TScalarType,NInputDimensions,NOutputDimensions>::Pointer RPCInverseFactory = RPCInverseTransformFactory::New();
itk::ObjectFactoryBase::RegisterFactory(RPCInverseFactory);
}
......
......@@ -30,7 +30,7 @@ namespace otb
template <class TScalarType, unsigned int NInputDimensions, unsigned int NOutputDimensions>
RPCInverseTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::RPCInverseTransformFactory()
{
this->RegisterOverride("otbSensorTransformBase", "otbRPCInverseTransform", "RPC Inverse Transform", 1,
this->RegisterOverride("otbRPCTransformBase", "otbRPCInverseTransform", "RPC Inverse Transform", 1,
itk::CreateObjectFunction<RPCInverseTransform<TScalarType,NInputDimensions,NOutputDimensions>>::New());
}
......
......@@ -19,11 +19,10 @@
*/
#ifndef otbSensorTransformFactory_h
#define otbSensorTransformFactory_h
#include "otbSensorTransformBase.h"
#include "itkObject.h"
#include "itkObjectFactory.h"
#include "itkMutexLock.h"
#include "otbSensorTransformBase.h"
#include "otbGenericMapProjection.h"
namespace otb
{
......@@ -55,11 +54,12 @@ protected:
SensorTransformFactory();
~SensorTransformFactory() override;
static itk::SimpleMutexLock m_mutex;
private:
SensorTransformFactory(const Self&) = delete;
void operator=(const Self&) = delete;
static itk::SimpleMutexLock m_mutex;
/** Register Built-in factories */
static void RegisterBuiltInFactories();
......
......@@ -21,20 +21,25 @@
#define otbSensorTransformFactory_hxx
#include "otbSensorTransformFactory.h"
#include "otbRPCTransformBase.h"
#include "otbRPCForwardTransformFactory.h"
#include "otbRPCInverseTransformFactory.h"
#include "otbRPCTransformBase.h"
#include "itkMutexLockHolder.h"
namespace otb
{
template <class TScalarType, unsigned int NInputDimensions, unsigned int NOutputDimensions>
itk::SimpleMutexLock SensorTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::m_mutex;
template <class TScalarType, unsigned int NInputDimensions, unsigned int NOutputDimensions>
typename SensorTransformBase<TScalarType, NInputDimensions,NOutputDimensions>::Pointer
SensorTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::CreateTransform(const ImageMetadata &imd,TransformDirection direction)
{
RegisterBuiltInFactories();
auto possibleobjects = itk::ObjectFactoryBase::CreateAllInstance("otbSensorTransformBase");
auto possibleobjects = itk::ObjectFactoryBase::CreateAllInstance("otbRPCTransformBase");
//TODO: add a list of possible SAR objects and concatenate rpc and sar list in one main list
for (auto && po : possibleobjects)
{
SensorTransformBase<TScalarType, NInputDimensions,NOutputDimensions>* io = dynamic_cast<SensorTransformBase<TScalarType, NInputDimensions,NOutputDimensions>*>(po.GetPointer());
......@@ -46,10 +51,7 @@ SensorTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::CreateT
return io;
}
}
else
{
otbLogMacro(Error, << "Error SensorTransform Factory did not return a SensorTransform: " << po->GetNameOfClass())
}
//io is null is possible because createAllInstance will return all known instances with different types like SensorTransform<double,2,2> SensorTransform<double,3,3> which has been registered before
}
return nullptr;
......@@ -58,10 +60,11 @@ SensorTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::CreateT
template <class TScalarType, unsigned int NInputDimensions, unsigned int NOutputDimensions>
void SensorTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::RegisterBuiltInFactories()
{
static std::once_flag initFlagForward,initFlagInverse;
std::call_once(initFlagForward,&SensorTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::RegisterFactory,RPCForwardTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::New());
std::call_once(initFlagInverse,&SensorTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::RegisterFactory,RPCInverseTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::New());
itk::MutexLockHolder<itk::SimpleMutexLock> lockHolder(m_mutex);
  • Just to be sure. I interpret this code as: "Executing the Register call several times make sense, but they should not happen concurrently" Is it really the intended behaviour?

    If on the contrary it shall be done only once, mutex are completely overkill, or in other words, expensive. And also, this code offer no protection. once appears to me to be a better solution. We could also rely on simple singletons, see for instance: otb::ogr::Drivers::Init()

    BTW, it seems the other RegisterBuiltInFactories() functions suffers from the same design issue.

    Actually given ObjectFactoryBase::RegisterFactory() returns a bool, we could simply store it in a static local variable. See https://en.cppreference.com/w/cpp/thread/call_once

    Initialization of function-local statics is guaranteed to occur only once even when called from multiple threads, and may be more efficient than the equivalent code using std::call_once.

    But as we need to wait... =>

    template <class TScalarType, unsigned int NInputDimensions, unsigned int NOutputDimensions>
    bool SensorTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::RegisterBuiltInFactories()
    {
        static bool registered = DoRegisterUnsafe();
        return registered; // in case we need to be sure the value isn't optimized  out
    }
    
    template <class TScalarType, unsigned int NInputDimensions, unsigned int NOutputDimensions>
    bool SensorTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::DoRegisterUnsafe()
    {
    
        return RegisterFactory(RPCForwardTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::New())
            && RegisterFactory(RPCInverseTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::New());
    }
    Edited by Luc Hermitte
  • You are right about the mutex, I think we should open an issue to refactor all the factories, to a less complex solution. I tried with once (in my previous commit), and I also tried with your solution static registerOnce but i found out that tests failed because the factory called was not the good one, thus the resulting RPC transform was not right. See below for an explanation

    "I interpret this code as: "Executing the Register call several times make sense, but they should not happen concurrently" Is it really the intended behaviour?" Yes, with the current design of factories in OTB, we have to call Register more than once (btw each time CreateTransform is called), because a new RPCTransform is getting created only when a new factory gets registered. With once it first creates a factory for one RPCTransform<double,2,2> for example, which returns the good object, but when a new call of CreateTransform appears (cf GenericRSTransform), it will not override the old registered factory as this factory is already created : std_once will not call a new Register so the RPCForward/InverseFactory will not return a new object. This results in createAllInstance in SensorTransformFactory returned list size being 0 because we have to call CleanFactories in the destructor of the previous GenericRSTransform to avoid access violations. I think this factory design in OTB is overkill because we use factories as a one timer object creator (for example CreateMachineLearningModel) instead of one factory instance to create objects multiple times.

    I did design this factory like the other one in OTB to have an homogenized behavior, but with a better overview now, this is not the best solution yet it is working. I will try using a singleton to create Transforms from a single instance

Please register or sign in to reply
//TODO: register SAR Inverse and Forward factories here
RegisterFactory(RPCForwardTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::New());
RegisterFactory(RPCInverseTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::New());
}
template <class TScalarType, unsigned int NInputDimensions, unsigned int NOutputDimensions>
......@@ -93,7 +96,7 @@ void SensorTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>::Cl
}
RPCInverseTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>* rpcInverseFactory =
dynamic_cast<RPCForwardTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>*>(*itFac);
dynamic_cast<RPCInverseTransformFactory<TScalarType, NInputDimensions,NOutputDimensions>*>(*itFac);
if (rpcInverseFactory)
{
itk::ObjectFactoryBase::UnRegisterFactory(rpcInverseFactory);
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment