Feat/segmentation service -> master #116
Conversation
…ions to ensure backward compatibility
| namespace api { | ||
| namespace pipeline { | ||
|
|
||
| /** |
There was a problem hiding this comment.
Use this syntax, otherwise doxygen does not generate the documentation properly:
/// Status of image segmentation processing
enum class ImageSegmentationStatus {
UNINITIALIZED, ///< processing not initialized
INITIALIZED, ///< processing correctly initialized, but not started
IN_PROGRESS, ///< processing in progress
COMPLETED, ///< processing completed
ABORTED ///< processing aborted before completion
};If you want to test, you'll also need this change I already merged to master:
d5b6492
Without it, enum declared in undocumented namespace are simply not visible in the documentation.
| * @enum class ImageSegmentationStatus | ||
| */ | ||
| enum class ImageSegmentationStatus { | ||
| UNINITIALIZED = 0, // processing not initialized |
There was a problem hiding this comment.
Also, no need to specify each value I think, because it's probably their default value anyway.
Plus we don't really use them.
| /// @brief segmentation request from an input image | ||
| /// @param[in] image pointer to image | ||
| /// @return FrameworkReturnCode::_SUCCESS (segmentation succeeded) or FrameworkReturnCode::_ERROR_ (segmentation failed) | ||
| virtual FrameworkReturnCode segmentationRequest(SRef<Image> image) = 0; |
There was a problem hiding this comment.
nit:
I know it's been done elsewhere, but not a fan of this name:
- usually a verb is used for method (action). Here it sounds like an object
- request is redundant. If we call a method, we make a request to the object to perform an action.
It could be for example: segment(), run/performSegmentation(), or something else.
If we want to be concise, it could even be just run()-, because calling IImageSegmentationPipeline::run() it is clear enough that run() will perform an image segmentation.
Also, I'm not sure, but enum ImageSegmentationStatut could be defined inside the class, and simply be called Status. The method getStatus() is not called getImageSegmentationStatus().
Just some thoughts, you decide ;)
| @@ -0,0 +1,120 @@ | |||
| /** | |||
| * @copyright Copyright (c) 2017-2025 B-com http://www.b-com.com/ | |||
There was a problem hiding this comment.
New file => 2025 only, no?
| namespace datastructure { | ||
|
|
||
| /** | ||
| * @enum class Segmentation2DType |
There was a problem hiding this comment.
See other comment about doxygen syntaxt for enums.
| return FrameworkReturnCode::_SUCCESS; | ||
| } | ||
|
|
||
| bool Mask2DCollection::isExistMask(uint32_t id) const |
There was a problem hiding this comment.
isExist is not really english. contains()?
| } | ||
| }; | ||
| using MaskInfoType = std::map<uint8_t, SegInfo>; | ||
| using ClassLabelType = std::map<int16_t, std::string>; |
There was a problem hiding this comment.
Shouldn't this table be moved out of Mask2D? I mean, for a given segmentation process, this map is going to be the same, right ?
Or maybe not? Is this a per image thing? But in theory, it could be interesting to use a single table for all masks to not have to repeat this association int->Class in every Mask2D?
MaskInfoType is probably tied to the Mask, because being limited to 255 values, it couldn't encode a whole image sequence, so this makes more sense.
It can stay as is for now I guess, but moving it outside could be an optimization to save some memory, if need be.
|
|
||
| /// @brief set Id | ||
| /// @param[in] id Mask2D ID | ||
| void setId(uint32_t id); |
There was a problem hiding this comment.
Are setters useful if the object can be entirely build with its ctor?
| return true; | ||
| } | ||
|
|
||
| SRef<const Mask2DCollection> Map::getConstMask2DCollection() const |
There was a problem hiding this comment.
Is it mandatory to return an SRef? Or a simple ref would suffice?
I.e. is there a scenario where the caller will claim the ownership of the collection, so that it can work on it while the Map is destroyed? Or is the collection lifetime linked to the one of the Map?
| if (defineMaskId) { | ||
| newMask->setId(m_currentId++); | ||
| } | ||
| m_masks[newMask->getId()] = newMask; |
There was a problem hiding this comment.
Thus do you think it's worth checking that the id does not already exist? Is it a warning or an error? Probably an error no?
No description provided.