MINT - Implement GPU accelerated nvJPEG encoder #20
Conversation
jestrada-atlassian
requested review from
a team,
sjhsieh,
jpujol,
w3sip,
ayushgr,
yiming-loom,
ncooleyy and
mchin3loom
and removed request for
a team
libavcodec/nvjpegenc.c
Outdated
| } | ||
|
|
||
| // Handle YUV input formats and populate nv_image data | ||
| if (sw_format == AV_PIX_FMT_YUV420P || sw_format == AV_PIX_FMT_YUVJ420P) { |
There was a problem hiding this comment.
Why do we want three different conditions / implementations here? Seems the same.
There was a problem hiding this comment.
Its a leftover from trying to get NV12 to work. Unfortunately couldn't get it to work. Will consolidate.
libavcodec/nvjpegenc.c
Outdated
| } | ||
|
|
||
| // Retrieve the bitstream again to populate output buffer | ||
| CHECK_NVJPEG(nvjpegEncodeRetrieveBitstream( |
libavcodec/nvjpegenc.c
Outdated
| NvjpegContext* ctx = avctx->priv_data; | ||
|
|
||
| if (ctx->encoder_state) { | ||
| CHECK_NVJPEG(nvjpegEncoderStateDestroy(ctx->encoder_state)); |
There was a problem hiding this comment.
Should we continue with cleanup even if one of these fails?
There was a problem hiding this comment.
Should be safe for now. We'll emit a log if this does happen but once we get to this point we'd have a valid output so I don't think we should throw an error even if this fails. We should still try to destroy the rest of the states if possible anyway.
There was a problem hiding this comment.
But CHECK_NVJPEG would return on error, right?
There was a problem hiding this comment.
You're right. Removed it from all these and just manually emitting a log
|
|
||
| ctx->width = avctx->width; | ||
| ctx->height = avctx->height; | ||
| if ((ret = nvjpeg_init(avctx)) < 0) |
There was a problem hiding this comment.
Personal nit: I usually try to avoid assignment and check of a condition in the same line. Depends on the codebase convention, though.
Another nit, I often prefer to have curly braces even on single-line scope. Again, depends on codebase convention.
There was a problem hiding this comment.
I'm roughly following the ffmpeg code convention found here. This implementation doesn't follow it perfectly but I'll clean it up before submitting the patch to them,
|
@jestrada-atlassian : do we expect a lot of gain here? even the software encoder for jpeg is extremely fast |
|
is this something we can submit upstream? It would be nice to have it reviewed by the nvidia guys |
I have this on my TODO list alongside getting |
libavcodec/nvjpegenc.c
Outdated
| } | ||
|
|
||
| // Retrieve the bitstream again to populate output buffer | ||
| ret = CHECK_NVJPEG(nvjpegEncodeRetrieveBitstream( |
There was a problem hiding this comment.
Remove CHECK_NVJPEG (still would return without freeing the buffer)
libavcodec/nvjpegenc.c
Outdated
| nv_image.channel[2] = frame->data[2]; | ||
| nv_image.pitch[2] = frame->linesize[2]; | ||
| } else if (input_format != NVJPEG_INPUT_TYPE_RGB) { | ||
| // Handle BGR/RGB input formats |
There was a problem hiding this comment.
I may be misunderstanding, bit either the comment is wrong, or != is? Could be totally misreading this, though.
There was a problem hiding this comment.
Simplified this. We're already checking the software format earlier so it doesn't make sense to check it here again
There was a problem hiding this comment.
Could simplify it further with
int planes = (input_format == NVJPEG_INPUT_TYPE_YUV) ? 3 : NVJPEG_MAX_COMPONENT;
and then
for (i = 0; i < planes; i++) {
nv_image.channel[i] = frame->data[i];
nv_image.pitch[i] = frame->linesize[i];
}
, up to you.
libavcodec/nvjpegenc.c
Outdated
| nv_image.channel[2] = frame->data[2]; | ||
| nv_image.pitch[2] = frame->linesize[2]; | ||
| } else if (input_format != NVJPEG_INPUT_TYPE_RGB) { | ||
| // Handle BGR/RGB input formats |
There was a problem hiding this comment.
Could simplify it further with
int planes = (input_format == NVJPEG_INPUT_TYPE_YUV) ? 3 : NVJPEG_MAX_COMPONENT;
and then
for (i = 0; i < planes; i++) {
nv_image.channel[i] = frame->data[i];
nv_image.pitch[i] = frame->linesize[i];
}
, up to you.
| ctx->nvjpeg_handle, ctx->encoder_state, out_buf, &out_buf_size, NULL); | ||
|
|
||
| if (ret != NVJPEG_STATUS_SUCCESS) { | ||
| av_free(out_buf); |
Adding a GPU accelerated jpeg encoder. This will allow us to accelerate parts of our transcoder thumbnail job that we previously have been unable to run on GPU.
https://www.loom.com/share/9ee806965ce244189b3de057b82dcae7