Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding video compression (h264) #44

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

bsobhani
Copy link

Starting pull request for video compression (h264) support. Not close to being perfect, but getting the process started so that it does not linger.

Basic functionality works and seems stable on the machines I tested it on. Achieves lower compression sizes with higher quality than existing codecs for some types of image sequences. For example, if the differences between frames is consistently small, like at SNS beamlines where neutron events accumulate on an image.

To compile, set:

WITH_VC=YES
VC_EXTERNAL=NO

To use, select "H264" from $(P)$(R)Compressor.

Tuning PVs:

  • $(P)$(R)QMinMax - 1 for least lossless compression, higher values for lossier compression
  • $(P)$(R)GOPSize - Group of picture size. 1 for frame-by-frame compression. Set to higher values for true video compression.

Copy link
Contributor

@ericonr ericonr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code and Makefiles still have a lot of comments and debugging printfs...

Comment on lines +1 to +7
Prerequisites:
* ffmpeg needs to be installed. "yum install ffmpeg-devel"

PVs:
* $(P)$(R)QMinMax - 1 for least lossless compression, higher values for lossier compression
* $(P)$(R)GOPSize - Group of picture size. 1 for frame-by-frame compression. Set to higher values for true video compression.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this PV documentation belong in NDPluginCodec?

And "1 for least lossy compression"

Comment on lines 11 to 19
AVCodecContext *c_c = 0;
int first = 1;
AVCodecContext *c_d = 0;
int count_d = 0;


const AVCodec *codec;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be best to avoid global variables? Doesn't this mean a single IOC can only have a single compressed stream?

Comment on lines +34 to +50
c->bit_rate = 400000;
c->width = width;
c->height = height;
//c->qmin = 1;
//c->qmax = 20;
c->time_base = (AVRational){1, 25};
c->framerate = (AVRational){25, 1};
//av_opt_set(c->priv_data, "crf", "1", AV_OPT_SEARCH_CHILDREN);
//av_opt_set(c->priv_data, "crf", "0", 0);
//av_opt_set(c->priv_data, "crf", "20", 0);
c->gop_size = 100;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these should be configurable by the users as well, no?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many parameters in libavcodec - I included only a couple which I thought were the most important in this initial commit. I think other parameters should be made configurable - it is just a matter of finding out what the important ones are from mine/others' experience.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like bit_rate, time_base and framerate at least should be 🤔

Comment on lines +54 to +63
if (ret < 0) {
fprintf(stderr, "Could not open codec: %s\n", av_err2str(ret));
exit(1);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failures in library code shouldn't cause the IOC to exit, right?

Comment on lines +126 to +147
else if (ret < 0) {
fprintf(stderr, "Error during encoding\n");
exit(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An encoding error shouldn't cause the IOC to exit.

Comment on lines +18 to +34
int H264_decompress(const char* source, char* dest, int originalSize)
{

int width, height;
decompress_buffer(source, originalSize, dest, &width, &height);
return width*height;

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function isn't declared in the header.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now that it's used in ADViewer. IMO it still should be in the header, as an exported interface.

extern "C" {
#endif

int H264_compress(const char* source, char* dest, int x_size, int y_size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the size for dest be passed to this function?

I'd also feel more comfortable with the type in use being unsigned char rather than char. Pixels won't have negative values after all.

Copy link
Author

@bsobhani bsobhani Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason H264_compress would need the size of dest would be if it did the checking to see if the dest buffer is large enough. The H264_compress function is currently implemented under the assumption that the dest buffer is large enough to store the compressed image. Whether it is large enough depends on the upper bound function being accurate enough (which I admit this is unknown to me), since this is what ensures that dest is large enough. So far I have not noticed issues here, but I will do testing to further investigate this.

Edit: It seems there is checking done by the compress_buffer function it calls. dest buffer size is calculated in that function by multiplying image dimensions. Then there is a check to make sure the compressed size is small enough before copying. I will do testing later.

@bsobhani
Copy link
Author

bsobhani commented Mar 6, 2024

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 and others added 7 commits April 1, 2024 12:28
…ome functions to take CodecContext object as a parameter
…context object as parameter instead of global context object. Also, make interfaces to these functions that take context as void pointer so that files that call these functions do not need to include libav headers
…sion, for compatibility.

yuv422 seems to have issues for certain setups (openh264). change to yuv420 until further investigation
made set_param_int function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants