View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0032979 | Open CASCADE | OCCT:Data Exchange | public | 2022-05-17 10:49 | 2022-08-05 21:15 |
Reporter | kgv | Assigned To | bugmaster | ||
Priority | normal | Severity | feature | ||
Status | reviewed | Resolution | open | ||
Target Version | 7.7.0 | ||||
Summary | 0032979: Data Exchange, RWGltf_CafWriter - support multi-threaded Draco compression | ||||
Description | Draco compression might be time-consuming on large models. Multi-threaded encoding might be helpful. | ||||
Tags | No tags attached. | ||||
Test case number | |||||
|
It might be useful to add the boolean option MultiThread to DracoParameters, and involve multi-threading only when it is on. |
|
Branch CR32979 has been created by ichesnokova. SHA-1: fb2c70cc7076988c1ca16e4486742dffc9df8c5b Detailed log of new commits: Author: ichesnok Date: Tue Jul 26 14:06:58 2022 +0300 0032979: Data Exchange, RWGltf_CafWriter - support multi-threaded Draco compression 'MultiThread' field was added to structure RWGltf_DracoParameters for using multithreading. Class CafWriter_DracoEncodingFunctor was added for multithreaded compression. |
|
Branch CR32979 has been updated forcibly by ichesnokova. SHA-1: f89bdae5d445bbe9d3dc36ec0df08164472f6489 |
|
Branch CR32979 has been updated forcibly by ichesnokova. SHA-1: 927e4f5632b464d86801ba9623d2505a93b672ca |
|
Branch CR32979 has been updated forcibly by ichesnokova. SHA-1: a00c3ac237e97f3d79b8dcdd87ca75fa619c282d |
|
Branch CR32979 is ready for review. Tests: http://jenkins-test-occt.nnov.opencascade.com/view/CR32979-master-ichesnok/view/ALL/ |
|
@ichesnokova, It could be useful adding at least one test case for verifying multi-threaded mode. Attaching performance comparison results would be also interesting. - aDracoEncoder.SetSpeedOptions (myDracoParameters.CompressionLevel, myDracoParameters.CompressionLevel); + aDracoEncoder.SetSpeedOptions(myDracoParameters.CompressionLevel, myDracoParameters.CompressionLevel); ... - Message::SendFail (TCollection_AsciiString("File '") + myBinFileNameFull + "' cannot be written"); + Message::SendFail(TCollection_AsciiString("File '") + myBinFileNameFull + "' cannot be written"); Please revert unrelated cosmetic changes and re-visit OCCT Coding Style suggestions (space is expected before opening bracket). + bool MultiThread; //!< flag to use multithreading (FALSE by default) Consider using consistent naming - see similar property in RWGltf_CafReader: > Standard_Boolean myToParallel; //!< flag to use multithreading; FALSE by default > . > //! Return TRUE if multithreaded optimizations are allowed; FALSE by default. > bool ToParallel() const { return myToParallel; } Not sure if it is for good or bad putting this flag into Draco-specific options instead of a `RWGltf_CafWriter`, @msv please share your suggestion. +class RWGltf_CafWriter::CafWriter_DracoEncodingFunctor +{ Cosmetic description would be useful. + draco::Mesh aMesh; + if (aCurrentMesh->NodesVec.empty()) + { + return; + } It looks strange to declare an unused variable `aMesh` before termination condition. +#include <OSD_ThreadPool.hxx> #include <OSD_Timer.hxx> Alphabetical order is preferred ('i' precedes 'h'). |
|
>Alphabetical order is preferred ('i' precedes 'h'). You are mistaking. 'h' precedes 'i'. |
|
@msv, my bad. |
|
src/RWGltf/RWGltf_CafWriter.hxx There is no necessity to put the class CafWriter_DracoEncodingFunctor into RWGltf_CafWriter. Please revert this change. src/RWGltf/RWGltf_CafWriter.cxx 174: Name the class DracoEncodingFunctor and put it into unnamed namespace. 179: This parameter should be const. 212: You are using default constructor, then copy constructor. Simplify as: std::shared_ptr<draco::EncoderBuffer> anEncoderBuffer = std::make_shared<draco::EncoderBuffer>(); 221: concurrent call of the method Next() will lead to data races. You must create a new progress range for each mesh item beforehand. Put mesh and progress range in a structure. 609: aPSentryBin is initialized only for 4 progress units, but now a new progress is added. 869: For this simple case it is better to use OSD_Parallel::For, like this: OSD_Parallel::For (0, int(aMeshes.size()), aFunctor, !myDracoParameters.MultiThread); |
|
Branch CR32979_1 has been created by ichesnokova. SHA-1: acf484e0082e69758aa259fe83b84da752fb398c Detailed log of new commits: Author: ichesnok Date: Tue Jul 26 14:06:58 2022 +0300 0032979: Data Exchange, RWGltf_CafWriter - support multi-threaded Draco compression 'MultiThread' field was added to structure RWGltf_DracoParameters for using multithreading. Class CafWriter_DracoEncodingFunctor was added for multithreaded compression. |
|
Branch CR32979_1 is ready for review. Tests: http://jenkins-test-occt.nnov.opencascade.com/view/CR32979-master-ichesnok/view/COMPARE/ performance.JPG (85,219 bytes) |
|
@ichesnokova, you may put preformatted ASCII table directly into comment using HTML tag 'pre'. |
|
Please organize progressing like in the class BRepExtrema_DistShapeShape.cxx |
|
Branch CR32979_1 has been updated forcibly by ichesnokova. SHA-1: dd9a6d2d3b2f7bef05a4c0e9bc9062f8cd95248f |
|
Branch CR32979_1 is ready for review. Tests: http://jenkins-test-occt.nnov.opencascade.com/view/CR32979-master-ichesnok/view/COMPARE/ performance_new.JPG (85,301 bytes) |
|
This is not needed any more:#include <OSD_ThreadPool.hxx> |
|
+ Standard_Boolean myToParallel; New class field is left uninitialized |
|
+ Message_ProgressScope aScope(theProgress, "Write binary data", 2); 2 should be conditional based on draco on/off. + if (!aStatus.ok()) + { + Message::SendFail (TCollection_AsciiString("Error: mesh cannot be encoded in draco buffer.")); + return; Failure should be propagated to the main interface. |
|
Branch CR32979_2 has been created by ichesnokova. SHA-1: 21d070b115c03e98bc1b474211aaadfe60241b69 Detailed log of new commits: Author: ichesnok Date: Tue Jul 26 14:06:58 2022 +0300 0032979: Data Exchange, RWGltf_CafWriter - support multi-threaded Draco compression 'MultiThread' field was added to structure RWGltf_DracoParameters for using multithreading. Class CafWriter_DracoEncodingFunctor was added for multithreaded compression. |
|
Branch CR32979_2 has been updated forcibly by ichesnokova. SHA-1: 403277678876c6d09ecff17ff5d2a8091e303577 |
|
Branch CR32979_2 is ready for review. Tests: http://jenkins-test-occt.nnov.opencascade.com/view/CR32979-master-ichesnok/view/ALL/ |
|
+ : myProgress(theProgress, "Convert meshes to Draco buffer", Max(1, int(theMeshes.size()))), Title looks unreasonably long to me. I would suggest "Draco compression" instead. + aBinFile->write ((*anEncoderBuffers.at(aBuffInd).get()).data(), std::streamsize((*anEncoderBuffers.at(aBuffInd).get()).size())); const draco::EncoderBuffer& anEncBuff = *anEncoderBuffers.at(aBuffInd); `aBinFile->write (anEncBuff.data(), std::streamsize(anEncBuff.size()));` |
|
Branch CR32979_2 has been updated forcibly by ichesnokova. SHA-1: ab9b10435378b00e10bbd8356fd29f302a190444 |
|
Branch CR32979_2 is ready for review. Tests: http://jenkins-test-occt.nnov.opencascade.com/view/CR32979-master-ichesnok/view/ALL/ |
|
For integration: occt - CR32979_2 products - none |
Date Modified | Username | Field | Change |
---|---|---|---|
2022-05-17 10:49 | kgv | New Issue | |
2022-05-17 10:49 | kgv | Assigned To | => ichesnokova |
2022-05-18 17:20 | msv | Note Added: 0108490 | |
2022-05-18 17:21 | msv | Status | new => assigned |
2022-07-26 14:08 | git | Note Added: 0109999 | |
2022-07-27 09:49 | git | Note Added: 0110013 | |
2022-07-28 08:53 | git | Note Added: 0110028 | |
2022-07-28 14:34 | git | Note Added: 0110055 | |
2022-07-29 16:34 | ichesnokova | Note Added: 0110075 | |
2022-07-29 16:34 | ichesnokova | Assigned To | ichesnokova => msv |
2022-07-29 16:34 | ichesnokova | Status | assigned => resolved |
2022-07-29 16:53 | kgv | Note Added: 0110078 | |
2022-07-29 17:15 | msv | Note Added: 0110082 | |
2022-07-29 17:21 | kgv | Note Added: 0110083 | |
2022-08-01 11:53 | msv | Note Added: 0110094 | |
2022-08-01 11:53 | msv | Assigned To | msv => ichesnokova |
2022-08-01 11:53 | msv | Status | resolved => assigned |
2022-08-02 10:44 | git | Note Added: 0110099 | |
2022-08-02 14:07 | ichesnokova | Note Added: 0110105 | |
2022-08-02 14:07 | ichesnokova | File Added: performance.JPG | |
2022-08-02 14:08 | ichesnokova | Assigned To | ichesnokova => msv |
2022-08-02 14:08 | ichesnokova | Status | assigned => resolved |
2022-08-02 14:17 | kgv | Note Added: 0110106 | |
2022-08-02 18:38 | msv | Note Added: 0110116 | |
2022-08-02 18:38 | msv | Assigned To | msv => ichesnokova |
2022-08-02 18:38 | msv | Status | resolved => assigned |
2022-08-03 10:01 | git | Note Added: 0110119 | |
2022-08-03 13:40 | ichesnokova | Note Added: 0110121 | |
2022-08-03 13:40 | ichesnokova | File Added: performance_new.JPG | |
2022-08-03 13:40 | ichesnokova | Assigned To | ichesnokova => msv |
2022-08-03 13:40 | ichesnokova | Status | assigned => resolved |
2022-08-03 14:02 | msv | Note Added: 0110122 | |
2022-08-03 14:02 | msv | Assigned To | msv => ichesnokova |
2022-08-03 14:02 | msv | Status | resolved => assigned |
2022-08-03 14:11 | kgv | Note Added: 0110123 | |
2022-08-03 14:15 | kgv | Note Added: 0110124 | |
2022-08-03 22:37 | git | Note Added: 0110128 | |
2022-08-04 08:13 | git | Note Added: 0110129 | |
2022-08-04 17:54 | ichesnokova | Note Added: 0110137 | |
2022-08-04 17:54 | ichesnokova | Assigned To | ichesnokova => msv |
2022-08-04 17:54 | ichesnokova | Status | assigned => resolved |
2022-08-05 09:50 | kgv | Note Added: 0110138 | |
2022-08-05 09:51 | kgv | Note Edited: 0110138 | |
2022-08-05 10:05 | msv | Assigned To | msv => ichesnokova |
2022-08-05 10:05 | msv | Status | resolved => assigned |
2022-08-05 10:54 | git | Note Added: 0110139 | |
2022-08-05 16:40 | ichesnokova | Note Added: 0110140 | |
2022-08-05 16:40 | ichesnokova | Assigned To | ichesnokova => msv |
2022-08-05 16:40 | ichesnokova | Status | assigned => resolved |
2022-08-05 21:15 | msv | Assigned To | msv => bugmaster |
2022-08-05 21:15 | msv | Status | resolved => reviewed |
2022-08-05 21:15 | msv | Note Added: 0110141 | |
2022-08-08 08:48 | ichesnokova | Time allocated | 8.8.2022: 166 h. => set |