Add OpenCL support #32

soreau · 2019-05-22T15:39:09Z

This attempts to use OpenCL to convert rgb data to yuv instead of sws_scale() when wf-recorder is built with OpenCL support and the --to-yuv or -t option is passed. This helps offload work to the gpu that would otherwise be done on the cpu.

ammen99

I like the idea

src/frame-writer.cpp

src/opencl.cpp

ammen99 · 2019-05-27T20:24:00Z

I think that the options now are a bit confusing TBH.

For example, we have a --no-opencl which works only with --convert-rgb, and we'd need --convert-rgb to enable OpenCL with libx264 (so GPU converting and CPU encoding), which is of course strange. What would you say about an option --opencl (which enables OpenCL if it can be used) and another, --force-yuv for the radeon vaapi usecase?

soreau · 2019-05-27T20:52:15Z

I think that the options now are a bit confusing TBH.

For example, we have a --no-opencl which works only with --convert-rgb, and we'd need
--convert-rgb to enable OpenCL with libx264 (so GPU converting and CPU encoding), which is of >course strange. What would you say about an option --opencl (which enables OpenCL if it can be >used) and another, --force-yuv for the radeon vaapi usecase?

I guess this is fine but in the case where opencl isn't built in, should we ifdef the option out? In the case where you'd use --force-yuv, I'm guessing you'd probably want to use opencl if it is built in, so you'd need to pass both options. You'd only need to disable opencl to test performance gains or if there's a problem.

I would like to have opencl enabled by default if it was built in and the other option could be called whatever but I would like --to-yuv.

soreau · 2019-05-28T10:25:49Z

I made it like you suggest to see how it works out. I meant -t as --to-yuv and -e as --enable-opencl but have --force-yuv and --opencl for now. I also committed your patch to use a function to avoid code duplication and literal string for the opencl program.

ammen99 · 2019-05-28T16:53:39Z

src/frame-writer.cpp

+    }
+#else
+    /* Silence compiler warning when opencl is disabled */
+    (void)(y_invert);


we can move this inside the #ifdef

The point of this is to not leave y_invert variable declared but not used in the case where opencl is disabled. If we put it inside the ifdef, the warning still happens.

ammen99 · 2019-05-28T16:54:29Z

src/main.cpp

        { 0,                 0,                 NULL,  0  }
    };

    int c, i;
    std::string param;
    size_t pos;
-    while((c = getopt_long(argc, argv, "o:f:g:c:p:d:la::t::", opts, &i)) != -1)
+    while((c = getopt_long(argc, argv, "o:f:g:c:p:d:la::t::e::", opts, &i)) != -1)


why does e have optional parameter when it doesn't take any value?

ammen99 · 2019-05-29T12:15:32Z

I'll test this later today and hopefully merge, if there is nothing else that you have planned to do.

soreau · 2019-05-29T14:54:51Z

I don't have anything else planned at this time.

soreau · 2019-06-03T22:58:49Z

@ammen99 Can you test this patch (on top of opencl branch)? Adjust the value of SIZE_MULTIPLIER and see if there is a relation to how much cpu is being used. I've tried between 1-32, with and without vaapi using libx264 and h264_vaapi. Here the results are that the higher the number (meaning the more memory allocated and copied), the less cpu that is being consumed (via htop). It seems counterintuitive that copying more data would result in less cpu usage. Is it the case for you too? Any idea what might be going on here?

soreau · 2019-06-11T10:14:49Z

As discussed in irc, it has been realized that the cpu was probably doing less because the gpu was doing more copying, essentially the same as calling usleep after copying. Now that I've set the buffers to the correct size, I've tested on a box with libx264 and h264_vaapi, with and without opencl, while idle and while moving mouse cursor, recording the top two (highest cpu usage) threads and the results are always better in the opencl case except libx264+sws_scale when idle. It also should now clean up properly without crashing.

                    |   sws_scale  |    OpenCL    |
                    |______________|______________|
                    |   T1   T2    |   T1   T2    |
____________________|______________|______________|
           | idle   |   95%  50%   |   97%  54%   |
libx264    |        |              |              |
           | motion |   57%  32%   |   51%  26%   |
___________|________|______________|______________|
           |        |              |              |
           | idle   |   47%  46%   |   38%  37%   |
h264_vaapi |        |              |              |
           | motion |   38%  38%   |   32%  32%   |

ammen99 · 2019-07-12T08:52:27Z

So this should be ready for merging, right? Could you please rebase on top of master?

OpenCL will be used for converting from rgb to yuv data when built with OpenCL support and using vaapi with to-yuv option.

Namely this avoids reallocating the output buffer each frame.

This makes it so OpenCL is used for rgb to yuv conversion instead of sws_scale(), when --to-yuv option is used.

And other stuff.

…ption The new yuv420p shader does pixel averaging to produce uv values from 2x2 subsample. It also supports odd size resolutions in the vaapi case. The --no-opencl option (or -n) disables opencl if it was built in.

soreau · 2019-07-12T19:24:17Z

So this should be ready for merging, right? Could you please rebase on top of master?

Yes, should be ready to go now.

ammen99 · 2019-08-05T20:48:31Z

I tested this final version with both OpenCL on and off, and both VAAPI on and off, seems to work fine.

Thanks for the work on this, and sorry for the delay.

soreau · 2019-08-05T22:26:51Z

I tested this final version with both OpenCL on and off, and both VAAPI on and off, seems to work fine.

Thanks for the work on this, and sorry for the delay.

No problem, thanks for testing. I'd like to see this 'fixed' in the gallium vaapi driver and test the internal converter for comparison, but maybe that will come in the future.

ammen99 requested changes May 23, 2019

View reviewed changes

src/frame-writer.cpp Outdated Show resolved Hide resolved

src/opencl.cpp Outdated Show resolved Hide resolved

ammen99 reviewed May 28, 2019

View reviewed changes

ammen99 mentioned this pull request Jun 14, 2019

wf-recorder --help / manpage #35

Merged

soreau added 18 commits July 12, 2019 12:58

Add optional OpenCL support

c9000ef

OpenCL will be used for converting from rgb to yuv data when built with OpenCL support and using vaapi with to-yuv option.

Various fixes

c96a739

Namely this avoids reallocating the output buffer each frame.

Warn when unfavorable dimensions are found

f4f9242

Add copyright headers to opencl source files

4c1e0ac

Choose hardware path for rgb to yuv conversion when applicable

fd07f0b

This makes it so OpenCL is used for rgb to yuv conversion instead of sws_scale(), when --to-yuv option is used.

Fix bug

9ddad65

Choose correct format when opencl is disabled and -t is specified

c93ee70

s/yuv/nv12/

d99dac4

Style nitpicks

f84d51c

Use yuv420p instead of nv12 when using sws_scale() since it is faster

99faeff

Fix memleak

b698fbc

And other stuff.

Get rid of some #ifdef nonsense

a81c014

Make ret variable private again

fdb6b0f

Use faster yuv420p when converting rgb in vaapi path without opencl

7d9befb

More error handling

1c71f0a

More error handling

e65a480

Fix pixel averaging

1f96239

soreau and others added 15 commits July 12, 2019 13:02

Fix vaapi encoding when opencl and sws_scale are not used

ed653af

Use string literal for opencl program

9e19cd5

Move duplicated code into function

4ebb6a3

Rename options

ad7e3eb

Remove debug messages

5a9b073

Couple fixups

6ebad8e

Fixups

7192114

Update README.md

fe3c2ae

Allow user to specify opencl device

4ef0ae7

Don't allocate more memory than needed

b620a5c

Actually, reading 4 x the data gives better perf without vaapi

889a972

Fix-up

4c67933

Remove which no longer causes segfault

cd01576

Update help and README.md

bd250b0

Fix mistake in help

6c1d4fa

soreau force-pushed the opencl branch from c357d51 to 6c1d4fa Compare July 12, 2019 19:22

ammen99 approved these changes Aug 5, 2019

View reviewed changes

ammen99 merged commit 1388f45 into ammen99:master Aug 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenCL support #32

Add OpenCL support #32

soreau commented May 22, 2019

ammen99 left a comment

ammen99 commented May 27, 2019

soreau commented May 27, 2019

soreau commented May 28, 2019

ammen99 May 28, 2019

soreau May 28, 2019

ammen99 May 28, 2019

soreau May 28, 2019

ammen99 commented May 29, 2019

soreau commented May 29, 2019

soreau commented Jun 3, 2019

soreau commented Jun 11, 2019

ammen99 commented Jul 12, 2019

soreau commented Jul 12, 2019

ammen99 commented Aug 5, 2019

soreau commented Aug 5, 2019

Add OpenCL support #32

Add OpenCL support #32

Conversation

soreau commented May 22, 2019

ammen99 left a comment

Choose a reason for hiding this comment

ammen99 commented May 27, 2019

soreau commented May 27, 2019

soreau commented May 28, 2019

ammen99 May 28, 2019

Choose a reason for hiding this comment

soreau May 28, 2019

Choose a reason for hiding this comment

ammen99 May 28, 2019

Choose a reason for hiding this comment

soreau May 28, 2019

Choose a reason for hiding this comment

ammen99 commented May 29, 2019

soreau commented May 29, 2019

soreau commented Jun 3, 2019

soreau commented Jun 11, 2019

ammen99 commented Jul 12, 2019

soreau commented Jul 12, 2019

ammen99 commented Aug 5, 2019

soreau commented Aug 5, 2019