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

Feature/1187 Read EXR files from memory #1448

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

nabielkandiel
Copy link

@nabielkandiel nabielkandiel commented May 31, 2024

Issue for reference
#1187

Below is an image of what currently happens when reading a .usda file in f3d
master

This is what it looks like on this branch
my_branch

file is pulled from here
https://github.com/mwestphal/assets/tree/main/full_assets/StandardShaderBall

Please note you have to change some options in the cmake build files to enable USD and EXR
CMakeLists.txt
line 58 turn on module_exr
plugins/CMakeLists.txt
line 6 turn on build_usd

MemStream is a private class created inside F3DEXRReader, this is needed as Imf::RgbaInputFile needs an input that has stream like functionality or is derived from Imf::IStream
https://openexr.com/en/latest/ReadingAndWritingImageFiles.html#low-level-i-o

I'm not sure if this is the best design choice to have it be a private class, i can change that if desired but im not sure if any other class is going to need to use this functionality.

Copy link

codecov bot commented Jun 1, 2024

Codecov Report

Attention: Patch coverage is 88.57143% with 4 lines in your changes missing coverage. Please review.

Project coverage is 96.65%. Comparing base (afe3870) to head (0f910f8).
Report is 9 commits behind head on master.

Files Patch % Lines
vtkext/private/module/vtkF3DEXRReader.cxx 88.57% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master    #1448    +/-   ##
========================================
  Coverage   96.64%   96.65%            
========================================
  Files         103      105     +2     
  Lines        7756     7861   +105     
========================================
+ Hits         7496     7598   +102     
- Misses        260      263     +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

#include <vector>
/**
* image file taken from
* https://github.com/AcademySoftwareFoundation/openexr/blob/370db2835843ac75f85e1386c05455f26a6ff58c/website/test_images/Chromaticities/Rec709.rst
Copy link
Contributor

Choose a reason for hiding this comment

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

im not sure how that .rst file is related to the .exr file ?

Copy link
Author

Choose a reason for hiding this comment

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

the rst file is just a markdown file on github showing the exr file and it details. I just grabbed it because it was smaller than the other exr files in the repo which made the unit test run a but faster.

https://github.com/AcademySoftwareFoundation/openexr/blob/370db2835843ac75f85e1386c05455f26a6ff58c/website/test_images/Chromaticities/Rec709.rst

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to just use an existing one. grayscale.exr for example is very small.

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed, better to use an existing one.

Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove this line, since you list it in data licenses now.

Copy link
Contributor

Choose a reason for hiding this comment

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

please remove this line

testing/data/Rec709.exr Outdated Show resolved Hide resolved
Copy link
Contributor

@Meakk Meakk left a comment

Choose a reason for hiding this comment

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

Here are some comments. But overall that looks good.

vtkext/private/module/Testing/TestF3DEXRMemReader.cxx Outdated Show resolved Hide resolved
application/testing/CMakeLists.txt Outdated Show resolved Hide resolved
testing/baselines/TestEXRReadMemory.png Outdated Show resolved Hide resolved
#include <vector>
/**
* image file taken from
* https://github.com/AcademySoftwareFoundation/openexr/blob/370db2835843ac75f85e1386c05455f26a6ff58c/website/test_images/Chromaticities/Rec709.rst
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to just use an existing one. grayscale.exr for example is very small.

@nabielkandiel
Copy link
Author

@Meakk I tried using grayscale.exr but i get the following errors

Error reading EXR file: only RGB and RGBA channels are supported
Could not find expected scalar array

(im not sure why but i couldn't reply to your original message in the thread)

@Meakk
Copy link
Contributor

Meakk commented Jun 3, 2024

@Meakk I tried using grayscale.exr but i get the following errors

Error reading EXR file: only RGB and RGBA channels are supported Could not find expected scalar array

(im not sure why but i couldn't reply to your original message in the thread)

Ok fair enough. Let's use your image then, but please double check the license and add it in testing/data/DATA_LICENSES.md

@Meakk
Copy link
Contributor

Meakk commented Jun 5, 2024

Small change required for cppcheck job:

/__w/f3d/f3d/source/vtkext/private/module/vtkF3DEXRReader.cxx:63:12: error: use default member initializer for 'pos' [modernize-use-default-member-init,-warnings-as-errors]
  uint64_t pos;
           ^
              {0}

*/
uint64_t tellg() override
{
return pos;
Copy link
Contributor

@Meakk Meakk Jun 5, 2024

Choose a reason for hiding this comment

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

Why isn't this line covered? Do we need to override this function if it's not called?

Copy link
Author

Choose a reason for hiding this comment

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

from this page
https://openexr.com/en/latest/ReadingAndWritingImageFiles.html#low-level-i-o
it seems like that function needs to be overriden to be a proper interface

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure to follow that answer

Copy link
Contributor

Choose a reason for hiding this comment

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

if that method is not used you can just remove it ?

Copy link
Author

Choose a reason for hiding this comment

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

it is a pure virtual function in the base class so i believe it needs an implementation. I can try removing it and see what happens. In the documentation i posted it says it needs an implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok lets try that. Maybe its needed in some case, but not in our case.

Copy link
Author

@nabielkandiel nabielkandiel Jun 18, 2024

Choose a reason for hiding this comment

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

it does not build without that function being implemented

error: cannot declare variable ‘memoryStream’ to be of abstract type ‘MemStream’
198 | MemStream memoryStream("EXRmemoryStream", this->MemoryBuffer, this->MemoryBufferLength);

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, lets keep it in for now.

@mwestphal mwestphal requested a review from Meakk June 16, 2024 17:09
@mwestphal
Copy link
Contributor

please rebase on master

: Imf::IStream(name)
, buffer(static_cast<const char*>(buff))
, bufflen(static_cast<size_t>(bufferLen))
, pos(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove this line

@@ -38,13 +88,15 @@ void vtkF3DEXRReader::ExecuteInformation()
this->ComputeInternalFileName(this->DataExtent[4]);
if (this->InternalFileName == nullptr || this->InternalFileName[0] == '\0')
{
return;
// If we have no file then maybe we have the file in memory
if (!this->MemoryBuffer)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you put this check in the line above ?


try
{
assert(this->InternalFileName);
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this assert should not be here for the memory stream part. BTW it may be better to make it an actual if test since we now can have an empty InteralFileName, I think.

Copy link
Author

Choose a reason for hiding this comment

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

Yes when loading a USD scene the EXRReader will have an empty filename. Interestingly in the test case TestF3DEXRMemReader, If i omit the filename on line 33 I get an error in vtkImageReader2.cxx:111 for not setting a filename.

*/
void vtkF3DEXRReader::SetMemoryBuffer(const void* buff)
{
MemoryBuffer = buff;
Copy link
Contributor

Choose a reason for hiding this comment

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

this->MemoryBuffer

Copy link
Contributor

Choose a reason for hiding this comment

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

this is the mother class implementation, why not just rely on it instead of defining your own ?

void vtkImageReader2::SetMemoryBuffer(const void* membuf)
{
  if (this->MemoryBuffer != membuf)
  {
    this->MemoryBuffer = membuf;
    this->Modified();
  }
}

//------------------------------------------------------------------------------
void vtkImageReader2::SetMemoryBufferLength(vtkIdType buflen)
{
  if (this->MemoryBufferLength != buflen)
  {
    this->MemoryBufferLength = buflen;
    this->Modified();
  }
}

Copy link
Author

Choose a reason for hiding this comment

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

I used this for debugging, it can now be removed.

@@ -53,6 +53,7 @@
- world*: VTK Data: BSD-3-Clause
- 16bits.*: @bisechen: [CC0](https://creativecommons.org/publicdomain/zero/1.0/)
- (ノಠ益ಠ )ノ.vtp: VTK Data: BSD-3-Clause
- Rec709.exr: [Copyright 2006 Industrial Light & Magic](https://github.com/AcademySoftwareFoundation/openexr/blob/370db2835843ac75f85e1386c05455f26a6ff58c/website/test_images/Chromaticities/Rec709.rst): BSD-3-Clause
Copy link
Contributor

Choose a reason for hiding this comment

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

please also add `small.usdz: Copyright USD contributors: CC-BY 4.0

Copy link
Contributor

Choose a reason for hiding this comment

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

with the links please

@@ -212,6 +207,10 @@ void vtkF3DEXRReader::ExecuteDataWithInformation(vtkDataObject* output, vtkInfor
}
else
{
if (!(this->InternalFileName))
{
throw std::invalid_argument("Not filename in EXR Reader when no Memory Buffer is set.");
Copy link
Contributor

Choose a reason for hiding this comment

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

can this happen ?

Do you really need to throw here ? you could just error out ?

@mwestphal
Copy link
Contributor

hey @nabielkandiel , need any help moving forward ?

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.

4 participants