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

ENH: Add vtkMRMLStreamingVolumeNode for compressed video #1028

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@Sunderlandkyl
Copy link

Sunderlandkyl commented Oct 11, 2018

  • vtkMRMLStreamingVolumeNode: Stores both compressed frames and uncompressed images. Decodes compressed frames into images and images to frames if requested using vtkStreamingVolumeCodec
  • vtkStreamingVolumeCodec: Handles encoding and decoding of images
  • vtkStreamingVolumeFrame: Stores compressed frame data along with the necessary information required to decode it (link to previous frame, uncompressed image size, codec FourCC, etc.)

Co-authored-by: Longquan Chen leochan2009@hotmail.com

Developed based on the changes made by @leochan2009 in #917

@pieper

This comment has been minimized.

Copy link
Member

pieper commented Oct 11, 2018

Looks nice. 👍

I wonder if the non-mrml code is a candidate for vtk proper or a vtk remote module. If so it could go into vtkAddons so people can use it in other vtk projects (vtkAddons is already set up as a vtk remote module).

Also how does this work with Sequences? Is it easy to convert back and forth?

It seems like we should add functionality at the vector volume node level to get a scalar volume version efficiently (basically add the pipelines from VectorToScalar Volume module)

@Sunderlandkyl Sunderlandkyl force-pushed the Sunderlandkyl:video_streaming branch from 8d200d1 to 6b42d04 Oct 11, 2018

@Sunderlandkyl

This comment has been minimized.

Copy link
Author

Sunderlandkyl commented Oct 11, 2018

That's true, the non MRML classes should be able to live in vtkAddons.

I've created another extension that I will publish soon that implements reading and writing MKV files using Sequences. Users can record and replay compressed video data that is streamed from OpenIGTLink and save it into an MKV. You can drag and drop any MKV file into Slicer as long as there is a supported codec registered.

Here's an example video showing replay in Slicer using a random MKV that I found (960x400, encoded using VP9):
https://www.dropbox.com/s/v9puhvkgjsavjhx/MKVReplaySlicer.mp4?dl=0

@leochan2009

This comment has been minimized.

Copy link

leochan2009 commented Oct 12, 2018

Hi Kyle,

Thanks for pushing the video streaming forward!

@Sunderlandkyl Sunderlandkyl force-pushed the Sunderlandkyl:video_streaming branch 2 times, most recently from d8af813 to 0d866cf Oct 12, 2018

@Sunderlandkyl

This comment has been minimized.

Copy link
Author

Sunderlandkyl commented Oct 15, 2018

The non MRML classes have been moved to vtkAddons.
Any other changes or suggestions?

@jcfr

This comment has been minimized.

Copy link
Member

jcfr commented Oct 15, 2018

As a side note, vtkFFMPEGVideoSource and vtkFFMPEGWriter were added to VTK few days ago, would that be of any use ?

See https://github.com/Kitware/VTK/tree/master/IO/FFMPEG

@lassoan

This comment has been minimized.

Copy link
Contributor

lassoan commented Oct 15, 2018

This is an API that will allow us to use any codec, including those that are built into VTK. The API can do much more than simply read frames from file and write to file: it supports random seeking, in-memory editing, recoding, and storing metadata.

Regarding VTK's FFMPEG support: FFMPEG is ridiculously large and complicated compared to what we need. We would need to a work a lot to make sure we disable all GPL parts and not use any questionable codecs in ffmpeg, figure out how to build ffmpeg with VP9 on all platforms, and access all the features through ffmpeg's API. So, for now we'll use VP9 codecs directly.

@Sunderlandkyl Sunderlandkyl force-pushed the Sunderlandkyl:video_streaming branch 2 times, most recently from 70a4c1a to 6246fed Oct 15, 2018

@lassoan
Copy link
Contributor

lassoan left a comment

Nice work! I added a number of comments inline, almost all of them just minor improvements or coding/naming style.

//----------------------------------------------------------------------------
vtkMRMLNodeNewMacro(vtkMRMLStreamingVolumeNode);

const int DEFAULT_NUMBER_OF_IMAGEDATACONNECTION_OBSERVERS = 1;

This comment has been minimized.

@lassoan

lassoan Oct 16, 2018

Contributor

Do we know which objects keep these observers? It would be nice to add it as comment.
It would be even better to determine this dynamically - can we check the number of observers right after creating the image and store that value?

vtkMRMLNodeNewMacro(vtkMRMLStreamingVolumeNode);

const int DEFAULT_NUMBER_OF_IMAGEDATACONNECTION_OBSERVERS = 1;
const int DEFAULT_NUMBER_OF_IMAGEDATA_OBSERVERS = 2;

This comment has been minimized.

@lassoan

lassoan Oct 16, 2018

Contributor

NUMBER_OF_INTERNAL_..._OBSERVERS would be better names

}

//---------------------------------------------------------------------------
bool vtkMRMLStreamingVolumeNode::IsImageObserved()

This comment has been minimized.

@lassoan

lassoan Oct 16, 2018

Contributor

It would be good if we could include the "external" word in this name, for example HasExternalImageObserver.

Show resolved Hide resolved Libs/MRML/Core/vtkMRMLStreamingVolumeNode.cxx
if (this->Frame)
{
vtkSmartPointer<vtkImageData> newImageData = vtkSmartPointer<vtkImageData>::New();
this->SetAndObserveImageData(newImageData);

This comment has been minimized.

@lassoan

lassoan Oct 16, 2018

Contributor

This may trigger updates everywhere where this node is observed (and those parts would then find a partially initialized/empty image data). We need to do all updates (preferably not just allocations but image content update) and set the new image data as the very last step.

//----------------------------------------------------------------------------
bool vtkStreamingVolumeCodecFactory::UnregisterStreamingCodecByClassName(const std::string& codecClassName)
{
for (unsigned int i = 0; i < this->RegisteredCodecs.size(); ++i)

This comment has been minimized.

@lassoan

lassoan Oct 16, 2018

Contributor

Why don't we use an iterator here?

/// \ingroup Volumes
/// \brief Class that can create compresion device for streaming volume instances.
///
/// This singleton class is a repository of all compression codecs for compressing volume .

This comment has been minimized.

@lassoan

lassoan Oct 16, 2018

Contributor
Suggested change Beta
/// This singleton class is a repository of all compression codecs for compressing volume .
/// This singleton class is a repository of all compression codecs for compressing volume.
vtkStreamingVolumeCodec* CreateCodecByFourCC(const std::string codecFourCC);

/// Returns a list of all registered Codecs
const std::vector<vtkSmartPointer<vtkStreamingVolumeCodec> >& GetStreamingCodecClasses();

This comment has been minimized.

@lassoan

lassoan Oct 16, 2018

Contributor

It would be a bit nicer and safer if we returned the class names instead of classes themselves

{
public:

enum

This comment has been minimized.

@lassoan

lassoan Oct 16, 2018

Contributor

add documentation, mention that it is for FrameType

vtkTypeMacro(vtkStreamingVolumeFrame, vtkObject);
void PrintSelf(ostream& os, vtkIndent indent) VTK_OVERRIDE;

vtkSetMacro(FrameType, int);

This comment has been minimized.

@lassoan

lassoan Oct 16, 2018

Contributor

Add documentation

This comment has been minimized.

@lassoan

lassoan Oct 17, 2018

Contributor

Could you please move documentation from member variables to get/set functions?
The reason is that when API documentation is generated, protected/private members (and their documentation) may not show up in the generated pages.

@Sunderlandkyl Sunderlandkyl force-pushed the Sunderlandkyl:video_streaming branch from 6246fed to 3b6afff Oct 17, 2018

@Sunderlandkyl

This comment has been minimized.

Copy link
Author

Sunderlandkyl commented Oct 17, 2018

@lassoan Changes made!

/// 3. virtual std::string UpdateParameterInternal(std::string parameterName, std::string parameterValue);
/// 4. virtual std::string GetFourCC();
/// 5. virtual std::string CreateCodecInstance(); // This can be overridden using vtkCodecNewMacro(className);
class VTK_ADDON_EXPORT vtkStreamingVolumeCodec : public vtkObject

This comment has been minimized.

@lassoan

lassoan Oct 17, 2018

Contributor

We should add a Raw or Trivial codec that just copies the data. It would serve as an example, it could be also used for testing, and for allowing reading/writing uncompressed data with the same API as compressed data.

/// For more information on frame types see: https://en.wikipedia.org/wiki/Video_compression_picture_types
enum
{
IFrame, // Uninterpolated keyframe

This comment has been minimized.

@lassoan

lassoan Oct 17, 2018

Contributor
Suggested change Beta
IFrame, // Uninterpolated keyframe
IFrame, ///< Uninterpolated keyframe
enum
{
IFrame, // Uninterpolated keyframe
PFrame, // Frame interpolated from previous frames

This comment has been minimized.

@lassoan

lassoan Oct 17, 2018

Contributor
Suggested change Beta
PFrame, // Frame interpolated from previous frames
PFrame, ///< Frame interpolated from previous frames
@lassoan
Copy link
Contributor

lassoan left a comment

Thank you, it is almost there. Apart from the small nitpicks there would be one more important thing to do: add a simple test that exercises basic mechanisms (it serves as an example and makes sure the classes work as intended). Sorry for keep asking things, I should have no more extra requests.

{
IFrame, // Uninterpolated keyframe
PFrame, // Frame interpolated from previous frames
BFrame, // Frame interpolated from previous and forward frames

This comment has been minimized.

@lassoan

lassoan Oct 17, 2018

Contributor
Suggested change Beta
BFrame, // Frame interpolated from previous and forward frames
BFrame, ///< Frame interpolated from previous and forward frames
vtkTypeMacro(vtkStreamingVolumeFrame, vtkObject);
void PrintSelf(ostream& os, vtkIndent indent) VTK_OVERRIDE;

vtkSetMacro(FrameType, int);

This comment has been minimized.

@lassoan

lassoan Oct 17, 2018

Contributor

Could you please move documentation from member variables to get/set functions?
The reason is that when API documentation is generated, protected/private members (and their documentation) may not show up in the generated pages.

vtkSetMacro(FrameType, int);
vtkGetMacro(FrameType, int);

void SetFrameData(vtkUnsignedCharArray* frameData) { this->FrameData = frameData; };

This comment has been minimized.

@lassoan

lassoan Oct 17, 2018

Contributor

need to call Modified()

void SetFrameData(vtkUnsignedCharArray* frameData) { this->FrameData = frameData; };
vtkUnsignedCharArray* GetFrameData() { return this->FrameData; };

void SetPreviousFrame(vtkStreamingVolumeFrame* previousFrame) { this->PreviousFrame = previousFrame; };

This comment has been minimized.

@lassoan

lassoan Oct 17, 2018

Contributor

need to call Modified()

@Sunderlandkyl Sunderlandkyl force-pushed the Sunderlandkyl:video_streaming branch from 3b6afff to d387cbd Oct 17, 2018

@Sunderlandkyl Sunderlandkyl force-pushed the Sunderlandkyl:video_streaming branch from d387cbd to 1ff2055 Oct 17, 2018

@Sunderlandkyl

This comment has been minimized.

Copy link
Author

Sunderlandkyl commented Oct 17, 2018

@lassoan Done!

vtkSmartPointer<vtkMRMLStreamingVolumeNode> streamingVolumeNode1 = vtkSmartPointer<vtkMRMLStreamingVolumeNode>::New();
streamingVolumeNode1->SetCodecFourCC("RV24");
streamingVolumeNode1->SetAndObserveImageData(imageData1);
if (streamingVolumeNode1->GetFrame())

This comment has been minimized.

@lassoan

lassoan Oct 17, 2018

Contributor

use CHECK_NULL(streamingVolumeNode1->GetFrame()); instead

}
streamingVolumeNode1->EncodeImageData();

vtkSmartPointer<vtkStreamingVolumeFrame> frameData = vtkSmartPointer<vtkStreamingVolumeFrame>::New();

This comment has been minimized.

@lassoan

lassoan Oct 17, 2018

Contributor

it is not useful to initialize frameData if you overwrite it in the next line


vtkSmartPointer<vtkStreamingVolumeFrame> frameData = vtkSmartPointer<vtkStreamingVolumeFrame>::New();
frameData = streamingVolumeNode1->GetFrame();
if (!frameData)

This comment has been minimized.

@lassoan

lassoan Oct 17, 2018

Contributor

use CHECK_NOT_NULL(frameData); instead


// Calling GetImageData should decode the frame
vtkSmartPointer<vtkImageData> imageData2 = streamingVolumeNode2->GetImageData();
if (!imageData2)

This comment has been minimized.

@lassoan

lassoan Oct 17, 2018

Contributor

check_not_null

EXERCISE_ALL_BASIC_MRML_METHODS(node1.GetPointer());


vtkSmartPointer<vtkStreamingVolumeCodecFactory> factory = vtkStreamingVolumeCodecFactory::GetInstance();

This comment has been minimized.

@lassoan

lassoan Oct 17, 2018

Contributor

We don't use factory explicitly. Is it necessary to instantiate it then? We can keep it but then use it (e.g., check if there are codecs registered, etc).

vtkStreamingVolumeFrame.h
vtkStreamingVolumeCodecFactory.cxx
vtkStreamingVolumeCodecFactory.h
vtkRGBVolumeCodec.cxx

This comment has been minimized.

@lassoan

lassoan Oct 17, 2018

Contributor

It looks like some actual codec. It would be better to include uncompressed, raw, trivial, simple, plain or some similar word in the name.

@Sunderlandkyl Sunderlandkyl force-pushed the Sunderlandkyl:video_streaming branch 3 times, most recently from 87496ee to 94606cd Oct 17, 2018

@Sunderlandkyl

This comment has been minimized.

Copy link
Author

Sunderlandkyl commented Oct 17, 2018

@lassoan Ok, changes made!

@lassoan

This comment has been minimized.

Copy link
Contributor

lassoan commented Oct 18, 2018

Thanks a lot @Sunderlandkyl - this looks good now!

@jcfr Would it be possible to include this in the Slicer-4.10.0?
It just adds a few new classes to the Slicer core so that extensions can register video codecs and other extensions can use them. It would allow us to offer video encoding in the 4.10 and avoid maintaining two branches of SlicerIGT, Sequences, and IGSIO repositories.

@jcfr

This comment has been minimized.

Copy link
Member

jcfr commented Oct 18, 2018

Would it be possible to include this in the Slicer-4.10.0?

Since the version has already been updated in the repo, and release on macOS and Windows is in progress. It will have to wait 4.10.1.

@Sunderlandkyl Sunderlandkyl force-pushed the Sunderlandkyl:video_streaming branch from 94606cd to d671664 Oct 18, 2018

@Sunderlandkyl

This comment has been minimized.

Copy link
Author

Sunderlandkyl commented Oct 18, 2018

@lassoan Added GetAvailiableParameterNames() and GetParameterDescription() functions to vtkStreamingVolumeCodec.

ENH: Add vtkMRMLStreamingVolumeNode for compressed video
- vtkMRMLStreamingVolumeNode: Stores both compressed frames and uncompressed images. Decodes compressed frames into images and images to frames if requested using vtkStreamingVolumeCodec
- vtkStreamingVolumeCodec: Handles encoding and decoding of images
- vtkStreamingVolumeFrame: Stores compressed frame data along with the necessary information required to decode it (link to previous frame, uncompressed image size, codec FourCC, etc.)

Co-authored-by: Longquan Chen <leochan2009@hotmail.com>

@Sunderlandkyl Sunderlandkyl force-pushed the Sunderlandkyl:video_streaming branch from d671664 to 72561ea Oct 23, 2018

@lassoan

This comment has been minimized.

Copy link
Contributor

lassoan commented Oct 25, 2018

Thank you very much for your contributions, it's been merged.

@lassoan lassoan closed this Oct 25, 2018

@Sunderlandkyl Sunderlandkyl deleted the Sunderlandkyl:video_streaming branch Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment