Optionally Have Segment Exporters Derive From AbstractSegmentExporter. #381
Conversation
| Path path = folder.resolve(shot.getId() + this.fileExtension); | ||
| OutputStream os = Files.newOutputStream(path); | ||
|
|
||
| /* Write audio data to OutputStream. */ |
There was a problem hiding this comment.
This is data-type agnostic at this point, no?
There was a problem hiding this comment.
Thanks for the PR. In my opinion there are some points to refactor / document. Also, since this change is particularly for the works in #379, I suggest to target that branch instead of main.
In addition to this, I do not fully understand what the Optionally in the title refers to?
| /** | ||
| * Destination path for the audio-segment. | ||
| */ | ||
| private final Path destination; |
There was a problem hiding this comment.
Why is this private object property documented and the following protected properties aren't?
I heavily suggest to document important information particularly on things that are exposed.
Our software stack has certainly room for improvement in terms of internal and external documentation and going forward, it seems to me it would be a good idea to aim for better documentation...
There was a problem hiding this comment.
I assume the path should be a folder? The documentation could be more precise.
| } | ||
|
|
||
| /** | ||
| * Default constructor. The AudioSegmentExport can be configured via named properties in the provided HashMap. |
There was a problem hiding this comment.
Copy-paster error: This isn't the AudioSegmentExporter anymore
| * Supported parameters: | ||
| * | ||
| * <ol> | ||
| * <li>destination: Path where files should be stored.</li> |
There was a problem hiding this comment.
I would highlight in the documentation that its literally destination
There was a problem hiding this comment.
Can you please clarify this?
There was a problem hiding this comment.
You have destination as the property key. I would expect that this is marked as such (e.g. with @code in line 57)
| * @param properties HashMap containing named properties | ||
| */ | ||
| public AbstractSegmentExporter(HashMap<String, String> properties) { | ||
| this.destination = Path.of(properties.getOrDefault(PROPERTY_NAME_DESTINATION, ".")); |
There was a problem hiding this comment.
I am not convinced that . is a sensible default here.
My suggestion would be $(pwd)/export to not fill the current working directory with individual files?
There was a problem hiding this comment.
I agree with this and am fixing this. Just for the record, originally AudioSegmentExporter did use '.'
...-core/src/main/java/org/vitrivr/cineast/core/features/abstracts/AbstractSegmentExporter.java
Show resolved
Hide resolved
|
|
||
| protected String fileExtension; | ||
|
|
||
| protected String dataUrlPrefix; |
There was a problem hiding this comment.
I would refactor this property to a protected abstract getter to force implementing classes to provide sensible values here
| if (!folder.toFile().exists()) { | ||
| folder.toFile().mkdirs(); | ||
| } | ||
| Path path = folder.resolve(shot.getId() + this.fileExtension); |
There was a problem hiding this comment.
No guarantee that this.fileExtension is set. Also there is no sanitation aboutfileExtension should include the delimiter (.) or not.
| String base64 = Base64.getEncoder().encodeToString(bytes); | ||
|
|
||
| // concatenate the data URL prefix and the Base64 string | ||
| return this.dataUrlPrefix + base64; |
There was a problem hiding this comment.
There is no guarantee at this point that this.dataUrlPrefix has been set
| public void init(PersistencyWriterSupplier phandlerSupply) { /* Nothing to init. */ } | ||
|
|
||
| @Override | ||
| public void finish() { /* Nothing to finish. */} |
There was a problem hiding this comment.
What about closing the stream?
There was a problem hiding this comment.
To put emphasis on this comment. I'd expect the finish method be abstract since the implementing exporter should flush and close its stream, not?
| this.width = Integer.parseInt(properties.getOrDefault(PROPERTY_NAME_WIDTH, "800")); | ||
| this.height = Integer.parseInt(properties.getOrDefault(PROPERTY_NAME_HEIGHT, "600")); | ||
| this.format = properties.getOrDefault(PROPERTY_NAME_FORMAT, "JPG"); | ||
| this.fileExtension = "." + format.toLowerCase(); |
There was a problem hiding this comment.
So, format has to be a valid extension?
|
For me it is useful to get some feedback from the experienced cineast developers for this change in isolation, as the ExternalApiExtraction branch does not make any changes to existing infrastructure. If this is not a vaid use for making a pull request, please let me know. "Optionally" refers to the fact that I am not making any breaking changes to existing exporters. They still work if they do not derive from AbstractSegmentExporter. If there is a need they can be refactored so that they derive from AbstractSegmentExporter and inherit additional functionality. Due to prioritization, I chose only to refactor two exporters for demonstration purposes. |
refactored the dataurl and fileextension to use getters
There was a problem hiding this comment.
Please excuse the late reply, this PR got lost in too many notifications. Thank you for the changes. I still think there is room for improvement.
Did you consider targeting the feature branch this work builds upon? In case you reject this suggestion I'd like to read about this in the discussion.
EDIT: Language
|
|
||
| protected static final Logger LOGGER = LogManager.getLogger(); | ||
|
|
||
| private static final String PROPERTY_NAME_DESTINATION = "destination"; |
There was a problem hiding this comment.
I'd prefer the property name to be public.
| /** | ||
| * Destination path for the audio-segment. | ||
| */ | ||
| private final Path destination; |
There was a problem hiding this comment.
I assume the path should be a folder? The documentation could be more precise.
| * Supported parameters: | ||
| * | ||
| * <ol> | ||
| * <li>destination: Path where files should be stored.</li> |
There was a problem hiding this comment.
You have destination as the property key. I would expect that this is marked as such (e.g. with @code in line 57)
| public void init(PersistencyWriterSupplier phandlerSupply) { /* Nothing to init. */ } | ||
|
|
||
| @Override | ||
| public void finish() { /* Nothing to finish. */} |
There was a problem hiding this comment.
To put emphasis on this comment. I'd expect the finish method be abstract since the implementing exporter should flush and close its stream, not?
| public void finish() { /* Nothing to finish. */} | ||
|
|
||
| @Override | ||
| public void initalizePersistentLayer(Supplier<EntityCreator> supply) { /* Nothing to init. */ } |
There was a problem hiding this comment.
Doesn't in your case mean persistent layer database + outgoing stream? In this case I'd expect this to beabstract such that the implementing exporter is able to performe proper initialisation.
For the upcoming features in the external-api-extraction branch, it would be useful to extract features from audio by sending a binary wav file data to the api. Exporting a wav file already exists in cineast, but the functionality was hard-coded to export to a local file and not an arbitrary data stream. To address this issue I refactored AudioSegmentExporter to inherit from AbstractSegmentExporter. This allows the programmer to only define how to export to a stream and then processSegment and other functions work automatically. I also implemented this on AudioSpectogramExporter as an example.