adding video compression (h264) #499

bsobhani · 2023-12-13T17:10:46Z

ADCore changes for areaDetector/ADSupport#44

ericonr

The existing code has a lot of leftover whitespace changes, commented out code, and debugging printfs. Are you planning on cleaning these up after initial review?

ericonr · 2024-03-06T13:52:54Z

ADApp/Db/NDCodec.template

+# Sets the quantizer (qmin and qmax) to value written to PV.
+# One can effectively adjust quality this way, with 1 being the least lossy, and higher numbers lossier
+record(ao, "$(P)$(R)QMinMax")
+{
+    field(DTYP, "asynInt32")
+    field(OUT,  "@asyn($(PORT),$(ADDR=0),$(TIMEOUT=1))QMINMAX")
+}


Why not allow users to set qmax and qmin separately?

It is my understanding that QMin and QMax set a range for acceptable "Q" (which stands for quantizer and roughly corresponds to quality), and then the codec selects a "Q" within this range based on various other factors. When I was testing this I found it useful to have a single PV that changes the compression quality, rather than separate min/max that must be set every time. It would be more complete to have separate QMin/QMax PVs, and then perhaps a PV that controls a "QCenter" which shifts QMin and QMax by the same amount as a convenience, although there is a little bit of complexity in implementing "QCenter", and I tried to keep things simple for the initial implementation. I agree that in principle QMin and QMax should be exposed.

Fair enough, that's a more complicated interface.

ericonr · 2024-03-06T13:56:47Z

ADApp/commonDriverMakefile

+ifeq ($(WITH_VC),YES)
+  ifeq ($(VC_EXTERNAL),NO)
+    PROD_LIBS += videoCompression
+  else
+    ifdef VC_LIB
+      VC_DIR = $(VC_LIB)
+      PROD_LIBS     += videoCompression
+    else
+      PROD_SYS_LIBS += videoCompression
+    endif
+  endif
+endif


I believe this is relevant here and for your PR in ADSupport. This videoCompression library seems to only be implemented in ADSupport. Is there a chance it could even be provided externally?

On a similar vein, if it isn't ever external, I don't think VC_LIB is required.

Good point.

ericonr · 2024-03-06T14:02:16Z

ADApp/pluginSrc/NDPluginCodec.cpp

+
+    NDArrayInfo_t info;
+    input->getInfo(&info);
+    int outputSize = LZ4_compressBound((int)info.totalBytes);


Is LZ4_compressBound the function that should be used here?

LZ4_compressBound appears to be a simple function that calculates a number slightly larger than the value that it is given. I found this sufficient when testing this plugin, but I will think about if another calculation (or perhaps simply using info.totalBytes) would be more suitable.

But LZ4_compressBound is also specific to LZ4 compression. That it works for video compression is more a coincidence than something that should be relied upon. That's why I think passing the buffer size to the compression function would at least allow us to catch issues with buffer size and avoid memory corruption. FFMpeg might also provide a function to help us determine the necessary buffer size, based on the configured bit-rate and quantization.

ericonr · 2024-03-06T14:04:52Z

ADApp/pluginSrc/NDPluginCodec.cpp

+NDArray *decompressH264(NDArray *input, NDCodecStatus_t *status, char *errorMessage)
+{
+    sprintf(errorMessage, "No H264 support");
+    *status = NDCODEC_ERROR;
+    return NULL;
+}
+
+
+void set_gop_size(int value){
+    printf("unable to set gop size to %d\n", value);
+}
+
+void set_q_min_max(int value){
+    printf("unable to set q min max to %d\n", value);
+}


These functions are implemented only for the case where HAVE_VC isn't defined.

I see now that set_gop_size and set_q_min_max are defined in the ADSupport functions. They are operating on global status; is it safe to operate on them during compression, for example? And this means we can't have more than one compression stream on the same IOC.

Is that part of the reason why the compress function can't run with unlock() called before it?

Yes I think a global variable is not ideal because I believe it is possible to have two separate cameras in a single IOC, and hence multiple instances of the plugin. I did not think of this. In practice, there is usually only camera per IOC. I don't think it is likely that the global variable is the reason for the unlock issues since I did my testing with only one instance of the plugin.

I don't think it is likely that the global variable is the reason for the unlock issues since I did my testing with only one instance of the plugin.

Even with a single instance of the plugin, but without the additional locks you added, if you write into the QMinMax or GOPSize PVs while compression is happening, you will reset the codec state while the struct is being used. That's not safe. These functions should be implemented in NDPluginCodec, which can track the current encoding state, and block writes to these values if encoding is ongoing.

ericonr · 2024-03-06T14:06:17Z

ADApp/pluginSrc/NDPluginCodec.cpp

+    createParam(NDCodecGOPSizeString, asynParamInt32,   &NDCodecGOPSize);
+    createParam(NDCodecQMinMaxString, asynParamInt32,   &NDCodecQMinMax);


This isn't following the existing formatting.

ericonr · 2024-03-06T14:06:33Z

ADApp/pluginSrc/NDPluginCodec.h

+#define NDCodecGOPSizeString  "GOP_SIZE" /* (int r/w) Group of pictures size for H264 */
+#define NDCodecQMinMaxString  "QMINMAX" /* (int r/w) Sets min and max values for quantizer for H264 */


This isn't following the existing formatting either.

bsobhani · 2024-03-06T18:11:16Z

Erico, thank you for the feedback. I have responded to some of your comments. When I have time I will take a more thorough look at the pull request and probably make some additional commits.

bsobhani · 2024-03-13T12:48:11Z

I did some testing and was able to confirm a couple of things: 1) If I create a mutex to prevent video compression parameters from being written while compressing then I can safely leave H264_compress unlocked in ADCore. 2) I tried running two video streams in the same IOC and confirm there were issues - if I make each instance of NDPluginCodec have its own video compression context pointer and pass this pointer to the video compression functions then the issues seem to go away.

I have made changes locally that seem to resolve these things, but not ready to push yet. Next week I should have some more time to work on this and then I will push my changes.

bsobhani · 2024-04-01T06:36:56Z

I have pushed changes which fix those two major issues. I plan to clean some things up and address some of the other smaller issues later.

Also, I seem to have done something to mutilate something in this pull request. All I was trying to do was fix a typo in my email address for a single commit. But in the process it seems to have changed the hashes of all the commits in the repo, which I think may be why it thinks there are 994 new commits. I will think about what to do about this.

Edit: I believe I have unmutilated the pull request. I will work on the other issues next (e.g. remove the use of the LZ4 bound function which creates a non-useful dependency on the LZ4 plugin, and look into framerate/bitrate/timebase), and will comment when I am finished.

…VC=NO

ericonr reviewed Mar 6, 2024

View reviewed changes

bsobhani force-pushed the video_compression branch from b97be0a to 59ea9c2 Compare April 1, 2024 05:59

bsobhani and others added 4 commits April 1, 2024 12:12

adding video compression (h264)

f90b86e

add null compress/decompress and null config functions for when WITH_…

aee675e

…VC=NO

uncomment lock/unlock

560dda6

make function calls to H264 compress functions take context pointer

5b4623e

bsobhani force-pushed the video_compression branch from 59ea9c2 to 5b4623e Compare April 1, 2024 16:14

MarkRivers marked this pull request as draft November 18, 2024 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding video compression (h264) #499

adding video compression (h264) #499

bsobhani commented Dec 13, 2023

ericonr left a comment

ericonr Mar 6, 2024

bsobhani Mar 6, 2024

ericonr Mar 8, 2024

ericonr Mar 6, 2024

bsobhani Mar 6, 2024

ericonr Mar 6, 2024

bsobhani Mar 6, 2024

ericonr Mar 8, 2024

ericonr Mar 6, 2024

ericonr Mar 6, 2024 •

edited

Loading

bsobhani Mar 6, 2024

ericonr Mar 8, 2024

ericonr Mar 6, 2024

ericonr Mar 6, 2024

bsobhani commented Mar 6, 2024

bsobhani commented Mar 13, 2024

bsobhani commented Apr 1, 2024 •

edited

Loading

		createParam(NDCodecGOPSizeString, asynParamInt32, &NDCodecGOPSize);
		createParam(NDCodecQMinMaxString, asynParamInt32, &NDCodecQMinMax);

		#define NDCodecGOPSizeString "GOP_SIZE" /* (int r/w) Group of pictures size for H264 */
		#define NDCodecQMinMaxString "QMINMAX" /* (int r/w) Sets min and max values for quantizer for H264 */

adding video compression (h264) #499

Are you sure you want to change the base?

adding video compression (h264) #499

Conversation

bsobhani commented Dec 13, 2023

ericonr left a 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

Choose a reason for hiding this comment

ericonr Mar 6, 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

bsobhani commented Mar 6, 2024

bsobhani commented Mar 13, 2024

bsobhani commented Apr 1, 2024 • edited Loading

ericonr Mar 6, 2024 •

edited

Loading

bsobhani commented Apr 1, 2024 •

edited

Loading