[FFmpeg-devel] [PATCH]opencl: compile kernels separately

[Lenny Wang]

On Wed, Oct 30, 2013 at 10:27 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Wed, Oct 30, 2013 at 03:29:40PM +0100, Stefano Sabatini wrote:
>> On date Wednesday 2013-10-30 12:01:57 +0100, Michael Niedermayer encoded:
>> > On Wed, Oct 30, 2013 at 11:07:20AM +0800, Wei Gao wrote:
>> [...]
>> > i suggest to split the patch into a libavutil and a libavfilter
>> > one. This reduces the chances of breaking public API/ABI as the
>> > code must still build (and work) with only the libavutil patch applied.
>> >
>> > also i suggest to make sure that your new API will serve future needs
>> > and be easy extendible not requiring major version bumps for changes.
>> > Its likely alot less work to
>> > design it well in the first place than to deal with these API/ABI
>> > compatibility issues later.
>> >
>> > old functions that are not part of the new API should be marked as
>> > deprecated, point the reader to the new and be put under #if so they
>> > get automatically disabled with the next major version bump.
>> >
>> > also if the new is still meant to be experimental, it might be best
>> > to require a --enable-experimental in configure.
>> > In that case then, random ABI/API changes would be no problem, sadly
>> > its too late for that for the old API, but it could be used for the
>> > new ...
>> >
>> > also, if its too hard to support old and new at the same time, one
>> > "hack" would be to support enough of the old API/ABI so building
>> > succeeds and code trying to use it gets a clear & clean error
>> > like return AVERROR(ENOSYS) on old functions.
>>
>> While I agree on --enable-experimental, this should not disallow to
>> break this API now, since it was agreed that it could be marked as
>> experimental one year ago.
>
> the problem is the ABI
>
> for example:
> libavfilter uses av_opencl_release_kernel()
> the patch removes this function from libavutil
>
> if someone has binary packages that have opencl enabled and he upgrades
> libavutil then libavfilter will have a reference to a function that
> no longer exists.
>
> and av_opencl_register_kernel_code() has a changed prototype in the
> patch its also used by libavfilter from avfilter_register_all()
> this will break every application using libavfilter no matter if they
> use opencl or not even if their linker doesnt freak out about the
> unresolved symbols before.
>
>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Democracy is the form of government in which you can choose your dictator
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Thanks for your comments.

I can keep the ABI intact without modifying
av_opencl_register_kernel_code() but still make this patch work as
intended.

I can also keep the old API but since their functionality will break,
I can do the "return AVERROR(ENOSYS) hack with a message" based on
Michael's suggestion.

The new API is a much better design than the old one and we wanted to
have this change long time ago.  We are pushing this now because we
also have two new opencl filters that requires the new API ready in
the commit queue.  The benefit of delayed kernel compilation allows us
to config the opencl kernels based on runtime parameter settings of a
filter.