Revision history [back]

Here is how I would refactor this code (in pseudo-C++)

class Node
{
    Node (ros::NodeHandle& nh, ros::NodeHandle& nhPrivate)
     // initialization list copy the arguments to the class attributes.
    {
      // create subscriber, publisher using nh_.
    }

    // always use shared pointer version of callbacks.
    subscriberCb (sensor_msgs::ImageShPtr& image) 
    {
       imageIn_ = image;
    }

    void process ()
    {
       // Use imageIn as your input image.
       CvImageConstPtr imageIn = imageIn_; // See note (1)

       // No new images since last time, do nothing.
       if (imageOut_.header.seq == imageIn.header.seq)
         return;

       // use imageOut_.image and images which are *class attributes*
       // to do realize the image processing, so it will allocate memory only once.
       // You should have *no* instance of big objects as local variables here.

       // publish imageOut_.toMsg ();
    }

    virtual void spin ()
    {
      ros::Rate rate (30);
      while (ros::ok ())
      {
        spinOnce ();
        rate.sleep ();
       }
    }

    virtual void spinOnce ()
    {
       process ();
       spinOnce ();
    }


protected:
  image_transport::ImageTransport it_;
  image_transport::Subscriber sub_;
  image_transport::Publisher pub_;
  ros::NodeHandle nh_;
  ros::NodeHandle nhPrivate_;

  CvImageConstPtr imageIn_;
  cv_bridge::CvImage imageOut_;
};

class NodeWithGui : public Node
{
    virtual void spinOnce ()
    {
       Node::spinOnce ();
       // and refresh GUI using imshow.
    }
};

int main ()
{
  ros::init (...);
  ros::NodeHandle nh;
  ros::NodeHandle nhPrivate ("~");

  Node* node = 0;
  if (ros::param::get<bool>("~use_gui"))
   node = new NodeWithGui (nh, nhPrivate);
  else
    node = new Node (nh, nhPrivate);
  node->spin ();
}

So the reason is not that you are doing your computation in the callback, the reason is that you reallocate at each callback all your objects!

To have an efficient node, allocation should be done once. At the beginning (here when the first image is received). Then, old structures should be reused.

Additional introduced features are:

• Pass the node handle and private node handle explicitly. It will help you convert your node to a nodelet one day (see nodelet conversion tutorial).
• ALWAYS separate code from GUI. Especially in robotics, robots should not embed X11 server.

Also, you can see that this code is much easier to benchmark as different piece of codes are separated. You can, for instance, bench how fast process is very easily.

Note (1): you may notice that before processing the data, I copy the shared pointer. This is to ensure that, if you use an ASyncSpinner, your image won't change in the middle of the processing. Shared pointers copies are atomic operations which means that this callback is thread-safe. This is required to convert this node to a nodelet for instance and also to deal with more complex GUI that may require you to use the ASyincSpinner.

Here is how I would refactor this code (in pseudo-C++)

class Node
{
    Node (ros::NodeHandle& nh, ros::NodeHandle& nhPrivate)
     // initialization list copy the arguments to the class attributes.
    {
      // create subscriber, publisher using nh_.
    }

    // always use shared pointer version of callbacks.
    subscriberCb (sensor_msgs::ImageShPtr& image) 
    {
       imageIn_ = image;
    }

    void process ()
    {
       // Use imageIn as your input image.
       CvImageConstPtr imageIn = imageIn_; // See note (1)

       // No new images since last time, do nothing.
       if (imageOut_.header.seq == imageIn.header.seq)
         return;

       // use imageOut_.image and images which are *class attributes*
       // to do realize the image processing, so it will allocate memory only once.
       // You should have *no* instance of big objects as local variables here.

       // publish imageOut_.toMsg ();
    }

    virtual void spin ()
    {
      ros::Rate rate (30);
      while (ros::ok ())
      {
        spinOnce ();
        rate.sleep ();
       }
    }

    virtual void spinOnce ()
    {
       process ();
       spinOnce ();
    }


protected:
  image_transport::ImageTransport it_;
  image_transport::Subscriber sub_;
  image_transport::Publisher pub_;
  ros::NodeHandle nh_;
  ros::NodeHandle nhPrivate_;

  CvImageConstPtr imageIn_;
  cv_bridge::CvImage imageOut_;
};

class NodeWithGui : public Node
{
    virtual void spinOnce ()
    {
       Node::spinOnce ();
       // and refresh GUI using imshow.
    }
};

int main ()
{
  ros::init (...);
  ros::NodeHandle nh;
  ros::NodeHandle nhPrivate ("~");

  Node* node = 0;
  if (ros::param::get<bool>("~use_gui"))
   node = new NodeWithGui (nh, nhPrivate);
  else
    node = new Node (nh, nhPrivate);
  node->spin ();
}

So the reason is not that you are doing your computation in the callback, the reason is that you reallocate at each callback all your objects!

To have an efficient node, allocation should be done once. At the beginning (here when the first image is received). Then, old structures should be reused.

Additional introduced features are:

• Pass the node handle and private node handle explicitly. It will help you convert your node to a nodelet one day (see nodelet conversion tutorial).
• ALWAYS separate code from GUI. Especially in robotics, robots should not embed X11 server.

Also, you can see that this code is much easier to benchmark as different piece of codes are separated. You can, for instance, bench how fast process is very easily.easily. Never use waitKey to get a "sensation" of how fast it is going. This is bad practice even if it is used in the cv_bridge tutorial. Do not use waitkey in your code. Use Timers to publish statistics through the ros logging api (rosconsole) about processing speed if you want to.

Note (1): you may notice that before processing the data, I copy the shared pointer. This is to ensure that, if you use an ASyncSpinner, your image won't change in the middle of the processing. Shared pointers copies are atomic operations which means that this callback is thread-safe. This is required to convert this node to a nodelet for instance and also to deal with more complex GUI that may require you to use the ASyincSpinner.

Here is how I would refactor this code (in pseudo-C++)

class Node
{
    Node (ros::NodeHandle& nh, ros::NodeHandle& nhPrivate)
     // initialization list copy the arguments to the class attributes.
    {
      // create subscriber, publisher using nh_.
    }

    // always use shared pointer version of callbacks.
    subscriberCb (sensor_msgs::ImageShPtr& image) 
    {
       imageIn_ = image;
    }

    void process ()
    {
       // Use imageIn as your input image.
       CvImageConstPtr imageIn = imageIn_; // See note (1)

       // No new images since last time, do nothing.
       if (imageOut_.header.seq == imageIn.header.seq)
         return;

       // use imageOut_.image and images which are *class attributes*
       // to do realize the image processing, so it will allocate memory only once.
       // You should have *no* instance of big objects as local variables here.
       // Use directly imageOut_.image as the cv::Mat destination object  of your last
       // OpenCV function call.

       // publish imageOut_.toMsg ();
    }

    virtual void spin ()
    {
      ros::Rate rate (30);
      while (ros::ok ())
      {
        spinOnce ();
        rate.sleep ();
       }
    }

    virtual void spinOnce ()
    {
       process ();
       spinOnce ();
    }


protected:
  image_transport::ImageTransport it_;
  image_transport::Subscriber sub_;
  image_transport::Publisher pub_;
  ros::NodeHandle nh_;
  ros::NodeHandle nhPrivate_;

  CvImageConstPtr imageIn_;
  cv_bridge::CvImage imageOut_;
};

class NodeWithGui : public Node
{
    virtual void spinOnce ()
    {
       Node::spinOnce ();
       // and refresh GUI using imshow.
    }
};

int main ()
{
  ros::init (...);
  ros::NodeHandle nh;
  ros::NodeHandle nhPrivate ("~");

  Node* node = 0;
  if (ros::param::get<bool>("~use_gui"))
   node = new NodeWithGui (nh, nhPrivate);
  else
    node = new Node (nh, nhPrivate);
  node->spin ();
}

So the reason is not that you are doing your computation in the callback, the reason is that you reallocate at each callback all your objects!

To have an efficient node, allocation should be done once. At the beginning (here when the first image is received). Then, old structures should be reused.

Additional introduced features are:

• Pass the node handle and private node handle explicitly. It will help you convert your node to a nodelet one day (see nodelet conversion tutorial).
• ALWAYS separate code from GUI. Especially in robotics, robots should not embed X11 server.
• No global variables.

Also, you can see that this code is much easier to benchmark as different piece of codes are separated. You can, for instance, bench how fast process is very easily. Never use waitKey to get a "sensation" of how fast it is going. This is bad practice even if it is used in the cv_bridge tutorial. Do not use waitkey in your code. Use Timers to publish statistics through the ros logging api (rosconsole) about processing speed if you want to.

Note (1): you may notice that before processing the data, I copy the shared pointer. This is to ensure that, if you use an ASyncSpinner, your image won't change in the middle of the processing. Shared pointers copies are atomic operations which means that this callback is thread-safe. This is required to convert this node to a nodelet for instance and also to deal with more complex GUI that may require you to use the ASyincSpinner.

Here is how I would refactor this code (in pseudo-C++)

class Node
{
    Node (ros::NodeHandle& nh, ros::NodeHandle& nhPrivate)
     // initialization list copy the arguments to the class attributes.
    {
      // create subscriber, publisher using nh_.
    }

    // always use shared pointer version of callbacks.
    subscriberCb (sensor_msgs::ImageShPtr& image) 
    {
       imageIn_ = image;
    }

    void process ()
    {
       // Use imageIn as your input image.
       CvImageConstPtr imageIn = imageIn_; // See note (1)

       // No new images since last time, do nothing.
       if (imageOut_.header.seq == imageIn.header.seq)
         return;

       // use imageOut_.image and images which are *class attributes*
       // to realize the image processing, so it will allocate memory only once.
       // You should have *no* instance of big objects as local variables here.
       // Use directly imageOut_.image as the cv::Mat destination object  of your last
       // OpenCV function call.

       // publish imageOut_.toMsg ();
    }

    virtual void spin ()
    {
      ros::Rate rate (30);
      while (ros::ok ())
      {
        spinOnce ();
        rate.sleep ();
       }
    }

    virtual void spinOnce ()
    {
       process ();
       spinOnce ();
    }


protected:
  image_transport::ImageTransport it_;
  image_transport::Subscriber sub_;
  image_transport::Publisher pub_;
  ros::NodeHandle nh_;
  ros::NodeHandle nhPrivate_;

  CvImageConstPtr imageIn_;
  cv_bridge::CvImage imageOut_;
};

class NodeWithGui : public Node
{
    virtual void spinOnce ()
    {
       Node::spinOnce ();
       // and refresh GUI using imshow.
    }
};

int main ()
{
  ros::init (...);
  ros::NodeHandle nh;
  ros::NodeHandle nhPrivate ("~");

  Node* node = 0;
  if (ros::param::get<bool>("~use_gui"))
   node = new NodeWithGui (nh, nhPrivate);
  else
    node = new Node (nh, nhPrivate);
  node->spin ();
}

So the reason is not that you are doing your computation in the callback, the reason is that you reallocate at each callback all your objects!

To have an efficient node, allocation should be done once. At the beginning (here when the first image is received). Then, old structures should be reused.

Additional introduced features are:

• Pass the node handle and private node handle explicitly. It will help you convert your node to a nodelet one day (see nodelet conversion tutorial).
• ALWAYS separate code algorithms from GUI. display (GUI). Especially in robotics, robots should not embed X11 server.
• No global variables.

Also, you can see that this code is much easier to benchmark as different piece of codes are separated. You can, for instance, bench how fast process is very easily. Never use waitKey to get a "sensation" of how fast it is going. This is bad practice even if it is used in the cv_bridge tutorial. Do not use waitkey in your code. Use Timers to publish statistics through the ros logging api (rosconsole) about processing speed if you want to.

Note (1): you may notice that before processing the data, I copy the shared pointer. This is to ensure that, if you use an ASyncSpinner, your image won't change in the middle of the processing. Shared pointers copies are atomic operations which means that this callback is thread-safe. This is required to convert this node to a nodelet for instance and also to deal with more complex GUI that may require you to use the ASyincSpinner.

Bonus points for advanced C++ developpers: template the Node class to realize different kind of spinning/scheduling using policy-based design for instance :)

Here is how I would refactor this code (in pseudo-C++)

class Node
{
    Node (ros::NodeHandle& nh, ros::NodeHandle& nhPrivate)
     // initialization list copy the arguments to the class attributes.
    {
      // create subscriber, publisher using nh_.
    }

    // always use shared pointer version of callbacks.
    subscriberCb (sensor_msgs::ImageShPtr& image) 
    {
       imageIn_ = image;
    }

    void process ()
    {
       // Use imageIn as your input image.
       CvImageConstPtr imageIn = imageIn_; // See note (1)

       // No new images since last time, do nothing.
       if (imageOut_.header.seq == imageIn.header.seq)
         return;

       // use imageOut_.image and images which are *class attributes*
       // to realize the image processing, so it will allocate memory only once.
       // You should have *no* instance of big objects as local variables here.
       // Use directly imageOut_.image as the cv::Mat destination object  of your last
       // OpenCV function call.

       // publish imageOut_.toMsg ();
    }

    virtual void spin ()
    {
      ros::Rate rate (30);
      while (ros::ok ())
      {
        spinOnce ();
        rate.sleep ();
       }
    }

    virtual void spinOnce ()
    {
       process ();
       spinOnce ();
    }


protected:
  image_transport::ImageTransport it_;
  image_transport::Subscriber sub_;
  image_transport::Publisher pub_;
  ros::NodeHandle nh_;
  ros::NodeHandle nhPrivate_;

  CvImageConstPtr imageIn_;
  cv_bridge::CvImage imageOut_;
};

class NodeWithGui : public Node
{
    virtual void spinOnce ()
    {
       Node::spinOnce ();
       // and refresh GUI using imshow.
    }
};

int main ()
{
  ros::init (...);
  ros::NodeHandle nh;
  ros::NodeHandle nhPrivate ("~");

  Node* node = 0;
  if (ros::param::get<bool>("~use_gui"))
   node = new NodeWithGui (nh, nhPrivate);
  else
    node = new Node (nh, nhPrivate);
  node->spin ();
}

So the reason is not that you are doing your computation in the callback, the reason is that you reallocate at each callback all your objects!

To have an efficient node, allocation should be done once. At the beginning (here when the first image is received). Then, old structures should be reused.

Additional introduced features are:

• Pass the node handle and private node handle explicitly. It will help you convert your node to a nodelet one day (see nodelet conversion tutorial).
• ALWAYS separate algorithms from display (GUI). Especially in robotics, robots should not embed X11 server.
• No global variables.

Also, you can see that this code is much easier to benchmark as different piece of codes are separated. You can, for instance, bench how fast process is very easily. Never use waitKey to get a "sensation" of how fast it is going. This is bad practice even if it is used in the cv_bridge tutorial. Do not use waitkey in your code. Use Timers to publish statistics through the ros logging api (rosconsole) about processing speed if you want to.

Note (1): you may notice that before processing the data, I copy the shared pointer. This is to ensure that, if you use an ASyncSpinner, your image won't change in the middle of the processing. Shared pointers copies are atomic operations which means that this callback is thread-safe. This is required to convert this node to a nodelet for instance and also to deal with more complex GUI that may require you to use the ASyincSpinner.ASyncSpinner. Note that no mutex are needed here, even in this case.

Bonus points for advanced C++ developpers: template the Node class to realize different kind of spinning/scheduling using policy-based design for instance :)

Here is how I would refactor this code (in pseudo-C++)

class Node
{
    Node (ros::NodeHandle& nh, ros::NodeHandle& nhPrivate)
     // initialization list copy the arguments to the class attributes.
    {
      // create subscriber, publisher using nh_.
    }

    // always use shared pointer version of callbacks.
    subscriberCb (sensor_msgs::ImageShPtr& (sensor_msgs::ImageConstPtr& image) 
    {
       imageIn_ = image;
    }

    void process ()
    {
       // Use imageIn as your input image.
       CvImageConstPtr imageIn = imageIn_; // See note (1)

       // No new images since last time, do nothing.
       if (imageOut_.header.seq == imageIn.header.seq)
         return;

       // use imageOut_.image and images which are *class attributes*
       // to realize the image processing, so it will allocate memory only once.
       // You should have *no* instance of big objects as local variables here.
       // Use directly imageOut_.image as the cv::Mat destination object  of your last
       // OpenCV function call.

       // publish imageOut_.toMsg ();
    }

    virtual void spin ()
    {
      ros::Rate rate (30);
      while (ros::ok ())
      {
        spinOnce ();
        rate.sleep ();
       }
    }

    virtual void spinOnce ()
    {
       process ();
       spinOnce ();
    }


protected:
  image_transport::ImageTransport it_;
  image_transport::Subscriber sub_;
  image_transport::Publisher pub_;
  ros::NodeHandle nh_;
  ros::NodeHandle nhPrivate_;

  CvImageConstPtr imageIn_;
  cv_bridge::CvImage imageOut_;
};

class NodeWithGui : public Node
{
    virtual void spinOnce ()
    {
       Node::spinOnce ();
       // and refresh GUI using imshow.
    }
};

int main ()
{
  ros::init (...);
  ros::NodeHandle nh;
  ros::NodeHandle nhPrivate ("~");

  Node* node = 0;
  if (ros::param::get<bool>("~use_gui"))
   node = new NodeWithGui (nh, nhPrivate);
  else
    node = new Node (nh, nhPrivate);
  node->spin ();
}

So the reason is not that you are doing your computation in the callback, the reason is that you reallocate at each callback all your objects!

To have an efficient node, allocation should be done once. At the beginning (here when the first image is received). Then, old structures should be reused.

Additional introduced features are:

• Pass the node handle and private node handle explicitly. It will help you convert your node to a nodelet one day (see nodelet conversion tutorial).
• ALWAYS separate algorithms from display (GUI). Especially in robotics, robots should not embed X11 server.
• No global variables.

Also, you can see that this code is much easier to benchmark as different piece of codes are separated. You can, for instance, bench how fast process is very easily. Never use waitKey to get a "sensation" of how fast it is going. This is bad practice even if it is used in the cv_bridge tutorial. Do not use waitkey in your code. Use Timers to publish statistics through the ros logging api (rosconsole) about processing speed if you want to.

Note (1): you may notice that before processing the data, I copy the shared pointer. This is to ensure that, if you use an ASyncSpinner, your image won't change in the middle of the processing. Shared pointers copies are atomic operations which means that this callback is thread-safe. This is required to convert this node to a nodelet for instance and also to deal with more complex GUI that may require you to use the ASyncSpinner. Note that no mutex are needed here, even in this case.

Bonus points for advanced C++ developpers: template the Node class to realize different kind of spinning/scheduling using policy-based design for instance :)

Here is how I would refactor this code (in pseudo-C++)

class Node
{
    Node (ros::NodeHandle& nh, ros::NodeHandle& nhPrivate)
     // initialization list copy the arguments to the class attributes.
    {
      // create subscriber, publisher using nh_.
    }

    // always use shared pointer version of callbacks.
    subscriberCb (sensor_msgs::ImageConstPtr& (const sensor_msgs::ImageConstPtr& image) 
    {
       imageIn_ = image;
    }

    void process ()
    {
       // Use imageIn as your input image.
       CvImageConstPtr sensor_msgs::ImageConstPtr imageIn = imageIn_; // See note (1)

       // No new images since last time, do nothing.
       if (imageOut_.header.seq == imageIn.header.seq)
         return;

       // use imageOut_.image and images which are *class attributes*
       // to realize the image processing, so it will allocate memory only once.
       // You should have *no* instance of big objects as local variables here.
       // Use directly imageOut_.image as the cv::Mat destination object  of your last
       // OpenCV function call.

       // publish imageOut_.toMsg ();
    }

    virtual void spin ()
    {
      ros::Rate rate (30);
      while (ros::ok ())
      {
        spinOnce ();
        rate.sleep ();
       }
    }

    virtual void spinOnce ()
    {
       process ();
       spinOnce ();
    }


protected:
  image_transport::ImageTransport it_;
  image_transport::Subscriber sub_;
  image_transport::Publisher pub_;
  ros::NodeHandle nh_;
  ros::NodeHandle nhPrivate_;

  CvImageConstPtr sensor_msgs::ImageConstPtr imageIn_;
  cv_bridge::CvImage imageOut_;
};

class NodeWithGui : public Node
{
    virtual void spinOnce ()
    {
       Node::spinOnce ();
       // and refresh GUI using imshow.
    }
};

int main ()
{
  ros::init (...);
  ros::NodeHandle nh;
  ros::NodeHandle nhPrivate ("~");

  Node* node = 0;
  if (ros::param::get<bool>("~use_gui"))
   node = new NodeWithGui (nh, nhPrivate);
  else
    node = new Node (nh, nhPrivate);
  node->spin ();
}

So the reason is not that you are doing your computation in the callback, the reason is that you reallocate at each callback all your objects!

To have an efficient node, allocation should be done once. At the beginning (here when the first image is received). Then, old structures should be reused.

Additional introduced features are:

• Pass the node handle and private node handle explicitly. It will help you convert your node to a nodelet one day (see nodelet conversion tutorial).
• ALWAYS separate algorithms from display (GUI). Especially in robotics, robots should not embed X11 server.
• No global variables.

Also, you can see that this code is much easier to benchmark as different piece of codes are separated. You can, for instance, bench how fast process is very easily. Never use waitKey to get a "sensation" of how fast it is going. This is bad practice even if it is used in the cv_bridge tutorial. Do not use waitkey in your code. Use Timers to publish statistics through the ros logging api (rosconsole) about processing speed if you want to.

Note (1): you may notice that before processing the data, I copy the shared pointer. This is to ensure that, if you use an ASyncSpinner, your image won't change in the middle of the processing. Shared pointers copies are atomic operations which means that this callback is thread-safe. This is required to convert this node to a nodelet for instance and also to deal with more complex GUI that may require you to use the ASyncSpinner. Note that no mutex are needed here, even in this case.

Bonus points for advanced C++ developpers: template the Node class to realize different kind of spinning/scheduling using policy-based design for instance :)