View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0031956Open CASCADEOCCT:Visualizationpublic2020-11-24 10:042023-04-14 14:58
Reporterkgv Assigned Torodrlyra  
PrioritynormalSeverityfeature 
Status resolvedResolutionopen 
Product Version7.4.0 
Target Version7.8.0 
Summary0031956: Visualization - provide Image_AlienPixMap::Save() writing into a memory buffer instead of a file
DescriptionImage_AlienPixMap::Load() provides overloads reading image from file path, memory buffer and std::istream. In contrast, Image_AlienPixMap::Save() is currently able to write only into the given file path, which restricts class usability.
Steps To Reproducetest v3d/bugs/bug31956
TagsNo tags attached.
Test case numberv3d bugs bug31956

Relationships

related to 0030549 resolvedabv Open CASCADE Coding - split Image_AlienPixMap into several classes 
child of 0030182 closedapn Open CASCADE Visualization, Image_AlienPixMap - support reading encoded image from memory buffer 

Activities

git

2021-09-14 14:12

administrator   ~0104074

Branch CR31956 has been created by CheskoArt.

SHA-1: fbf48c08439946f24061d1b7d2f6b0bb78ed656a


Detailed log of new commits:

Author: achesnok
Date: Mon Sep 6 14:36:40 2021 +0300

    0031956: Visualization - provide Image_AlienPixMap::Save() writing into a memory buffer instead of a file
    
    Added two new Image_AlienPixMap::Save() overloads, taking std::ostream or memory buffer arguments.

git

2021-09-15 21:34

administrator   ~0104104

Branch CR31956 has been updated forcibly by CheskoArt.

SHA-1: 58887fde1aeca0408c1dc7f872b53386626370a0

git

2021-09-15 22:04

administrator   ~0104105

Branch CR31956 has been updated forcibly by CheskoArt.

SHA-1: c5594d863afbdf330ae4dae912be6ca5133d74b2

CheskoArt

2021-09-16 19:07

developer   ~0104131

Please review the patch.
Test results: http://jenkins-test-occt.nnov.opencascade.com/view/CR31956-master-achesnok/view/ALL/

kgv

2021-09-16 19:23

developer   ~0104132 

+#ifdef HAVE_FREEIMAGE
+  Standard_EXPORT FIBITMAP* GetImageToDump (int theFormat);
   FIBITMAP* myLibImage;

This breaks binary compatibility and should be reverted.
HAVE_FREEIMAGE should not occur in .hxx header.

git

2021-09-16 21:05

administrator   ~0104134

Branch CR31956 has been updated forcibly by CheskoArt.

SHA-1: 5303e6b270b86fbcd8a769b5fab4fa2e653c0604

git

2021-09-18 14:36

administrator   ~0104215

Branch CR31956 has been updated forcibly by CheskoArt.

SHA-1: db9b3fca3d8c457239a50b13e33c5ae5bf9dc868

CheskoArt

2021-09-18 17:37

developer   ~0104217

Patch fixed

szy

2022-10-04 16:52

manager   ~0111388

Review request.

vpozdyayev

2022-10-10 11:35

administrator   ~0111449

Last edited: 2022-10-10 11:39

 
-: myLibImage (NULL)
+#ifdef HAVE_FREEIMAGE
+  : myLibImage (NULL)
+#endif

Won't this break ''Image_AlienPixMap::Clear''? This feature needs validation for all build types.

+  //!                    when theData isn't NULL used only to determine format

What's "theData"?

+  //! @param theBuffer   buffer pointer where to write

What are the NULL buffer semantics here?

+  Standard_EXPORT bool Save (const TCollection_AsciiString& theFileName,
+                             Standard_Byte* theBuffer = NULL,
+                             Standard_Size theLength = 0);

Of all the Save/Load functions here, this is the only one that does not take filename as the last argument. Let's not sacrifice API consistency to avoid adding one more function.

git

2022-11-17 10:16

administrator   ~0112141

Branch CR31956_1 has been created by mzernova.

SHA-1: db9b3fca3d8c457239a50b13e33c5ae5bf9dc868


No new revisions were added by this update.

git

2022-11-17 10:52

administrator   ~0112143

Branch CR31956 has been updated forcibly by mzernova.

SHA-1: 2307f70538bb6f13358f6e14f8cdc3ad63277f00

git

2022-11-17 11:02

administrator   ~0112144

Branch CR31956 has been updated forcibly by mzernova.

SHA-1: b49c83d4330962d7a20c3d0dc0f27f7b6c33b843

git

2022-11-17 11:02

administrator   ~0112145

Branch CR31956_1 has been updated forcibly by mzernova.

SHA-1: f327716896c5dddfb5c22d8f370598c16ace25dd

mzernova

2022-11-18 06:21

developer   ~0112161

http://jenkins-test-occt.nnov.opencascade.com/view/CR31956_1-master-mzernova/view/ALL/

mzernova

2022-11-18 06:22

developer   ~0112162

OCCT branch: CR31956_1

dpasukhi

2023-02-09 14:24

developer   ~0113099

Deat mzernova
Please update branch and retest fix

git

2023-02-10 13:32

administrator   ~0113106

Branch CR31956_1 has been updated forcibly by mzernova.

SHA-1: 189127fc4a95cb163317d2faeb865c1ce73d370b

git

2023-02-15 15:26

administrator   ~0113148

Branch CR31956_1 has been updated forcibly by mzernova.

SHA-1: fad361b45ee627dc645fc8aa5c27dcf87d4dc60c

mzernova

2023-02-15 15:36

developer   ~0113149

Dear rodrlyra,
please review OCCT - CR31956_1

Tests:
http://jenkins-test-10.nnov.opencascade.com/view/CR31956-master-mzernova/view/COMPARE/
(Some tests fail due to jenkins issues.)

New test case was created.

dpasukhi

2023-02-15 16:06

developer   ~0113150

Dear @mzernova
There a few style remarks, please check (they are just recommendations)

I think commens is not fully. You implement two methods that working with Path and stream, if i understand correctle
Added two new Image_AlienPixMap::Save() overloads, taking std::ostream or memory buffer arguments.

I think we use a little differ doxygen style
@param[in]
@param[out]
+  //! Write image data to file or memory buffer using file extension to determine format.
+  //! @param theBuffer   buffer pointer where to write
+  //!                    when NULL, function write image data to theFileName file
+  //! @param theLength   memory buffer length
+  //! @param theFileName file name to save;
+  //!                    when theBuffer isn't NULL used only to determine format

Not changable parameters should be const
+  Standard_EXPORT bool Save (Standard_Byte* theBuffer,
+                             Standard_Size theLength,
+                             const TCollection_AsciiString& theFileName);

Private methods should not be STANDARD_EXPORT.
not changable parameter shoud be const.
Something wrong with spaces separating Private: and methods and fields.
Best practices are using privete fields and protected methods.
+private:
+  Standard_EXPORT FIBITMAP* GetImageToDump (int theFormat);
   FIBITMAP* myLibImage;

What is it? Why You do not use Messager::SendFaile or theDI <<
There more cases then I can explane below
+  if (ViewerTest::CurrentView().IsNull())
+  {
+    std::cout << "Error: no active view\n";
+    return 1;
+  }
...
+    else
+    {
+      std::cout << "Syntax error at '" << anArg << "'\n";
+      return 1;
+    }
...
+  if (aFile.get() == NULL)
+  {
+    std::cout << "Syntax error: image file '" << anImgPath << "' cannot be found\n";
+    return 1;
+  }

rodrlyra

2023-02-27 18:48

developer   ~0113211

Dear @mzernova,
taking from Dmitry remarks, I would reinforce some corrections I think is necessary:

1 - Change '@param' from the documentation to '@param[in]' if it is input parameter, or '@param[out]' if output.

2 - Delete the 'Standard_EXPORT' from private method.

3 - On QABugs_1, the use of std::cout indeed does not seem correct. Taking the example of other QABugs, the output should be written to the 'Draw_Interpretor' parameter.

Lastly, I have seen that the new test you created tests two overloads of the Save() function: using stream, and using buffer. I think it is also important to add a test to the overload that saves to a file path.

git

2023-04-05 10:48

administrator   ~0113362

Branch CR31956_1 has been updated forcibly by mzernova.

SHA-1: 3209a916243e50bcd7d7146e044d4e2a45332656

git

2023-04-05 18:38

administrator   ~0113367

Branch CR31956_1 has been updated by mzernova.

SHA-1: 4f1308d94e2e11fbde5930bb716294261284597a


Detailed log of new commits:

Author: mzernova
Date: Wed Apr 5 16:37:52 2023 +0100

    # remarks

git

2023-04-05 18:50

administrator   ~0113368

Branch CR31956_1 has been updated forcibly by mzernova.

SHA-1: b961a59c42ee9f834b30d9c5a407cae75e78413c

mzernova

2023-04-14 14:58

developer   ~0113383

Last edited: 2023-04-14 14:58

 Dear rodrlyra,
please review OCCT - CR31956_1

Tests:
http://jenkins-test-10.nnov.opencascade.com/view/CR31956-master-mzernova/view/COMPARE/
(Version for Windows and some tests fail due to jenkins issues.)

Also I don't think that we need an additional test to save the image to a file path. This is old functionality and is used to dump images in many tests.

Issue History

Date Modified Username Field Change
2020-11-24 10:04 kgv New Issue
2020-11-24 10:04 kgv Assigned To => kgv
2020-11-24 10:06 kgv Relationship added related to 0030182
2020-11-24 10:06 kgv Relationship replaced child of 0030182
2020-11-24 10:06 kgv Product Version => 7.4.0
2020-11-24 10:06 kgv Relationship added related to 0030549
2021-08-11 11:31 age Assigned To kgv => CheskoArt
2021-08-11 11:53 CheskoArt Status new => assigned
2021-09-14 14:12 git Note Added: 0104074
2021-09-15 21:34 git Note Added: 0104104
2021-09-15 22:04 git Note Added: 0104105
2021-09-16 19:07 CheskoArt Note Added: 0104131
2021-09-16 19:07 CheskoArt Assigned To CheskoArt => kgv
2021-09-16 19:07 CheskoArt Status assigned => resolved
2021-09-16 19:07 CheskoArt Steps to Reproduce Updated
2021-09-16 19:23 kgv Note Added: 0104132
2021-09-16 21:05 git Note Added: 0104134
2021-09-18 08:04 CheskoArt Assigned To kgv => CheskoArt
2021-09-18 08:04 CheskoArt Status resolved => assigned
2021-09-18 14:36 git Note Added: 0104215
2021-09-18 17:37 CheskoArt Note Added: 0104217
2021-09-18 17:37 CheskoArt Assigned To CheskoArt => kgv
2021-09-18 17:37 CheskoArt Status assigned => resolved
2021-09-18 17:37 CheskoArt Steps to Reproduce Updated
2021-09-19 10:01 kgv Target Version 7.6.0 => 7.7.0
2022-10-04 16:52 szy Assigned To kgv => vpozdyayev
2022-10-04 16:52 szy Note Added: 0111388
2022-10-10 11:35 vpozdyayev Note Added: 0111449
2022-10-10 11:35 vpozdyayev Note Edited: 0111449
2022-10-10 11:36 vpozdyayev Note Edited: 0111449
2022-10-10 11:36 vpozdyayev Note Edited: 0111449
2022-10-10 11:38 vpozdyayev Note Edited: 0111449
2022-10-10 11:39 vpozdyayev Note Edited: 0111449
2022-10-10 15:54 vpozdyayev Assigned To vpozdyayev => mzernova
2022-10-10 15:56 vpozdyayev Status resolved => assigned
2022-10-24 10:43 szy Target Version 7.7.0 => 7.8.0
2022-11-17 10:16 git Note Added: 0112141
2022-11-17 10:52 git Note Added: 0112143
2022-11-17 11:02 git Note Added: 0112144
2022-11-17 11:02 git Note Added: 0112145
2022-11-18 06:21 mzernova Note Added: 0112161
2022-11-18 06:22 mzernova Note Added: 0112162
2022-11-18 06:23 mzernova Assigned To mzernova => vpozdyayev
2022-11-18 06:23 mzernova Status assigned => resolved
2022-11-18 06:23 mzernova Steps to Reproduce Updated
2023-01-24 19:21 eugeny.belousov_162739 Assigned To vpozdyayev => bugmaster
2023-02-09 14:24 dpasukhi Assigned To bugmaster => mzernova
2023-02-09 14:24 dpasukhi Status resolved => assigned
2023-02-09 14:24 dpasukhi Note Added: 0113099
2023-02-10 13:32 git Note Added: 0113106
2023-02-15 15:26 git Note Added: 0113148
2023-02-15 15:36 mzernova Assigned To mzernova => rodrlyra
2023-02-15 15:36 mzernova Status assigned => resolved
2023-02-15 15:36 mzernova Note Added: 0113149
2023-02-15 15:36 mzernova Test case number => v3d/bugs/bug31956
2023-02-15 15:37 mzernova Test case number v3d/bugs/bug31956 => v3d bugs bug31956
2023-02-15 16:06 dpasukhi Note Added: 0113150
2023-02-27 18:48 rodrlyra Note Added: 0113211
2023-02-27 18:49 rodrlyra Assigned To rodrlyra => mzernova
2023-02-27 18:49 rodrlyra Status resolved => assigned
2023-04-05 10:48 git Note Added: 0113362
2023-04-05 18:38 git Note Added: 0113367
2023-04-05 18:50 git Note Added: 0113368
2023-04-14 14:58 mzernova Assigned To mzernova => rodrlyra
2023-04-14 14:58 mzernova Status assigned => resolved
2023-04-14 14:58 mzernova Note Added: 0113383
2023-04-14 14:58 mzernova Note Edited: 0113383