Feature/1187 Read EXR files from memory #1448

nabielkandiel · 2024-05-31T22:50:34Z

Issue for reference
#1187

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

This is what it looks like on this 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.

codecov · 2024-06-01T01:05:29Z

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.

vtkext/private/module/vtkF3DEXRReader.h

vtkext/private/module/vtkF3DEXRReader.cxx

vtkext/private/module/Testing/CMakeLists.txt

mwestphal · 2024-06-02T10:20:22Z

vtkext/private/module/Testing/TestF3DEXRMemReader.cxx

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


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

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

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

indeed, better to use an existing one.

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

please remove this line

testing/data/Rec709.exr

vtkext/private/module/vtkF3DEXRReader.cxx

Meakk

Here are some comments. But overall that looks good.

vtkext/private/module/Testing/TestF3DEXRMemReader.cxx

application/testing/CMakeLists.txt

testing/baselines/TestEXRReadMemory.png

Meakk · 2024-06-03T07:03:51Z

vtkext/private/module/Testing/TestF3DEXRMemReader.cxx

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


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

nabielkandiel · 2024-06-03T08:00:49Z

@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 · 2024-06-03T09:19:57Z

@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 · 2024-06-05T18:33:23Z

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}

Meakk · 2024-06-05T18:34:36Z

vtkext/private/module/vtkF3DEXRReader.cxx

+   */
+  uint64_t tellg() override
+  {
+    return pos;


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

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

not sure to follow that answer

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

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.

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

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);

ok, lets keep it in for now.

vtkext/private/module/vtkF3DEXRReader.cxx

mwestphal · 2024-06-22T15:42:11Z

please rebase on master

mwestphal · 2024-06-23T07:13:44Z

vtkext/private/module/vtkF3DEXRReader.cxx

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


you can remove this line

mwestphal · 2024-06-23T07:17:04Z

vtkext/private/module/vtkF3DEXRReader.cxx

@@ -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)


can you put this check in the line above ?

mwestphal · 2024-06-23T07:17:43Z

vtkext/private/module/vtkF3DEXRReader.cxx

+
+  try
+  {
+    assert(this->InternalFileName);


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.

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.

mwestphal · 2024-06-23T07:18:16Z

vtkext/private/module/vtkF3DEXRReader.cxx

+ */
+void vtkF3DEXRReader::SetMemoryBuffer(const void* buff)
+{
+  MemoryBuffer = buff;


this->MemoryBuffer

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(); } }

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

mwestphal · 2024-06-23T08:17:38Z

testing/data/DATA_LICENSES.md

@@ -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


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

with the links please

mwestphal · 2024-06-27T05:58:14Z

vtkext/private/module/vtkF3DEXRReader.cxx

@@ -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.");


can this happen ?

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

mwestphal · 2024-07-13T16:23:19Z

hey @nabielkandiel , need any help moving forward ?

nabielkandiel added 5 commits May 30, 2024 21:36

began implementation

a9d19fa

Merge branch 'f3d-app:master' into feature/1187-exr-from-memory

ec8dc6c

remove debug

e38c111

clean up file

2e0ab33

fixed CI issues

35e9c9f

Meakk reviewed Jun 1, 2024

View reviewed changes

vtkext/private/module/vtkF3DEXRReader.h Outdated Show resolved Hide resolved

nabielkandiel added 2 commits June 2, 2024 02:00

moved class to cpp file and added unit tests

5f9ae1a

add comment with image file source

c4a65f0

mwestphal reviewed Jun 2, 2024

View reviewed changes

vtkext/private/module/vtkF3DEXRReader.cxx Outdated Show resolved Hide resolved

mwestphal reviewed Jun 2, 2024

View reviewed changes

vtkext/private/module/Testing/CMakeLists.txt Show resolved Hide resolved

mwestphal reviewed Jun 2, 2024

View reviewed changes

testing/data/Rec709.exr Outdated Show resolved Hide resolved

mwestphal reviewed Jun 2, 2024

View reviewed changes

vtkext/private/module/vtkF3DEXRReader.cxx Outdated Show resolved Hide resolved

nabielkandiel added 2 commits June 2, 2024 08:00

remove unused function

6d66a8f

add integration tests

69dc5e8

Meakk reviewed Jun 3, 2024

View reviewed changes

add ARGS in cmake test

d15f044

Meakk reviewed Jun 5, 2024

View reviewed changes

nabielkandiel added 3 commits June 12, 2024 19:39

default initialize value

88622bc

add license for testing data

4ea8564

change return for test

a9a6c81

mwestphal requested a review from Meakk June 16, 2024 17:09

mwestphal reviewed Jun 16, 2024

View reviewed changes

vtkext/private/module/vtkF3DEXRReader.cxx Outdated Show resolved Hide resolved

nabielkandiel added 2 commits June 19, 2024 21:58

update formatting

ef76977

update tests

82e2b40

nabielkandiel added 2 commits June 22, 2024 20:57

update tests

767eec2

Merge branch 'master' into feature/1187-exr-from-memory

e513d7a

mwestphal reviewed Jun 23, 2024

View reviewed changes

Cleanup

0f910f8

mwestphal reviewed Jun 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/1187 Read EXR files from memory #1448

Feature/1187 Read EXR files from memory #1448

nabielkandiel commented May 31, 2024 •

edited

Loading

codecov bot commented Jun 1, 2024 •

edited

Loading

mwestphal Jun 2, 2024

nabielkandiel Jun 2, 2024

Meakk Jun 3, 2024

mwestphal Jun 3, 2024

mwestphal Jun 23, 2024

mwestphal Jun 27, 2024

Meakk left a comment

Meakk Jun 3, 2024

nabielkandiel commented Jun 3, 2024

Meakk commented Jun 3, 2024

Meakk commented Jun 5, 2024

Meakk Jun 5, 2024 •

edited

Loading

nabielkandiel Jun 12, 2024

mwestphal Jun 16, 2024

mwestphal Jun 16, 2024

nabielkandiel Jun 16, 2024

mwestphal Jun 17, 2024

nabielkandiel Jun 18, 2024 •

edited

Loading

mwestphal Jun 22, 2024

mwestphal commented Jun 22, 2024

mwestphal Jun 23, 2024

mwestphal Jun 23, 2024

mwestphal Jun 23, 2024

nabielkandiel Jun 26, 2024

mwestphal Jun 23, 2024

mwestphal Jun 23, 2024

nabielkandiel Jun 26, 2024

mwestphal Jun 23, 2024

mwestphal Jun 27, 2024

mwestphal Jun 27, 2024

mwestphal commented Jul 13, 2024

Feature/1187 Read EXR files from memory #1448

Are you sure you want to change the base?

Feature/1187 Read EXR files from memory #1448

Conversation

nabielkandiel commented May 31, 2024 • edited Loading

codecov bot commented Jun 1, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Meakk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabielkandiel commented Jun 3, 2024

Meakk commented Jun 3, 2024

Meakk commented Jun 5, 2024

Meakk Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabielkandiel Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwestphal commented Jun 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwestphal commented Jul 13, 2024

nabielkandiel commented May 31, 2024 •

edited

Loading

codecov bot commented Jun 1, 2024 •

edited

Loading

Meakk Jun 5, 2024 •

edited

Loading

nabielkandiel Jun 18, 2024 •

edited

Loading