Re: [FFmpeg-devel] [PATCH] avutil/qsv: move ff_qsv_error function from libavcodec into libavutil, because it's going to be shared between libavcodec (existing QSV encoders & decoders), libavfilter (up

[Mark Thompson]

On 13/04/16 09:18, nablet developer wrote:
> Signed-off-by: nablet developer 
> ---
>  libavcodec/qsv.c  | 35 +---
>  libavcodec/qsv_internal.h |  5 
>  libavcodec/qsvdec.c   |  1 +
>  libavcodec/qsvenc.c   |  1 +
>  libavutil/Makefile|  1 +
>  libavutil/qsv_internal.c  | 58 
> +++
>  libavutil/qsv_internal.h  | 27 ++
>  7 files changed, 89 insertions(+), 39 deletions(-)
>  create mode 100644 libavutil/qsv_internal.c
>  create mode 100644 libavutil/qsv_internal.h
> 
> diff --git a/libavutil/qsv_internal.h b/libavutil/qsv_internal.h
> new file mode 100644
> index 000..de00d09
> --- /dev/null
> +++ b/libavutil/qsv_internal.h
> ...
> +/**
> +  * Convert a libmfx error code into a ffmpeg error code.
> +  */
> +int ff_qsv_error(int mfx_err);

This fails for non-static builds because of the namespace prefix (try building
the shared libraries).

Does this function really need to be available everywhere?  I think you should
wait until you have other patches which actually require it to so that this
change can be assessed properly.  In isolation, it is not useful.

Also, you should have a look at your mail setup.  This arrived today dated six
weeks ago - either it has taken unreasonably long to arrive or something strange
is going on at your sending machine.

Thanks,

- Mark

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avutil/qsv: move ff_qsv_error function from libavcodec into libavutil, because it's going to be shared between libavcodec (existing QSV encoders & decoders), libavfilter (up

[nablet developer]

> On 24 May 2016, at 17:01, Mark Thompson  wrote:
> 
> On 13/04/16 09:18, nablet developer wrote:
>> Signed-off-by: nablet developer 
>> ---
>> libavcodec/qsv.c  | 35 +---
>> libavcodec/qsv_internal.h |  5 
>> libavcodec/qsvdec.c   |  1 +
>> libavcodec/qsvenc.c   |  1 +
>> libavutil/Makefile|  1 +
>> libavutil/qsv_internal.c  | 58 
>> +++
>> libavutil/qsv_internal.h  | 27 ++
>> 7 files changed, 89 insertions(+), 39 deletions(-)
>> create mode 100644 libavutil/qsv_internal.c
>> create mode 100644 libavutil/qsv_internal.h
>> 
>> diff --git a/libavutil/qsv_internal.h b/libavutil/qsv_internal.h
>> new file mode 100644
>> index 000..de00d09
>> --- /dev/null
>> +++ b/libavutil/qsv_internal.h
>> ...
>> +/**
>> +  * Convert a libmfx error code into a ffmpeg error code.
>> +  */
>> +int ff_qsv_error(int mfx_err);
> 
> This fails for non-static builds because of the namespace prefix (try building
> the shared libraries).

oh, good catch, thanks. I think function then should be called 
"avpriv_qsv_error" according to the 
https://ffmpeg.org/developer.html#Naming-conventions 
, right?
I will also test my further changes with shared builds starting from now (I was 
using instructions from https://trac.ffmpeg.org/wiki/CompilationGuide/Centos 
 which were for static 
build).

> 
> Does this function really need to be available everywhere?  I think you should
> wait until you have other patches which actually require it to so that this
> change can be assessed properly.  In isolation, it is not useful.

hm, if my other patches will depend on this patch, how can they be applied 
before this change? I am actually following advice from 
https://ffmpeg.org/developer.html#Submitting-patches-1 
 to split patches into 
small self-contained pieces. please advice how to proceed.

> 
> Also, you should have a look at your mail setup.  This arrived today dated six
> weeks ago - either it has taken unreasonably long to arrive or something 
> strange
> is going on at your sending machine.

e-mail was sent today - I was using CentOS development virtual machine which 
had no ntp installed, now it's corrected. sorry for the inconveniences.

> 
> Thanks,
> 
> - Mark
> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avutil/qsv: move ff_qsv_error function from libavcodec into libavutil, because it's going to be shared between libavcodec (existing QSV encoders & decoders), libavfilter (up

[Mark Thompson]

On 24/05/16 12:49, nablet developer wrote:
> 
>> On 24 May 2016, at 17:01, Mark Thompson  wrote:
>>
>> On 13/04/16 09:18, nablet developer wrote:
>>> Signed-off-by: nablet developer 
>>> ---
>>> libavcodec/qsv.c  | 35 +---
>>> libavcodec/qsv_internal.h |  5 
>>> libavcodec/qsvdec.c   |  1 +
>>> libavcodec/qsvenc.c   |  1 +
>>> libavutil/Makefile|  1 +
>>> libavutil/qsv_internal.c  | 58 
>>> +++
>>> libavutil/qsv_internal.h  | 27 ++
>>> 7 files changed, 89 insertions(+), 39 deletions(-)
>>> create mode 100644 libavutil/qsv_internal.c
>>> create mode 100644 libavutil/qsv_internal.h
>>>
>>> diff --git a/libavutil/qsv_internal.h b/libavutil/qsv_internal.h
>>> new file mode 100644
>>> index 000..de00d09
>>> --- /dev/null
>>> +++ b/libavutil/qsv_internal.h
>>> ...
>>> +/**
>>> +  * Convert a libmfx error code into a ffmpeg error code.
>>> +  */
>>> +int ff_qsv_error(int mfx_err);
>>
>> This fails for non-static builds because of the namespace prefix (try 
>> building
>> the shared libraries).
> 
> oh, good catch, thanks. I think function then should be called 
> "avpriv_qsv_error" according to the 
> https://ffmpeg.org/developer.html#Naming-conventions 
> , right?

Correct, assuming this form is accepted.

(For example, it could instead be a static inline function and avoid the
namespace issue by not appearing as a symbol in the binary.  Not sure whether
that is actually better, but it is a possibility to consider.)

> I will also test my further changes with shared builds starting from now (I 
> was using instructions from 
> https://trac.ffmpeg.org/wiki/CompilationGuide/Centos 
>  which were for static 
> build).
> 
>>
>> Does this function really need to be available everywhere?  I think you 
>> should
>> wait until you have other patches which actually require it to so that this
>> change can be assessed properly.  In isolation, it is not useful.
> 
> hm, if my other patches will depend on this patch, how can they be applied 
> before this change? I am actually following advice from 
> https://ffmpeg.org/developer.html#Submitting-patches-1 
>  to split patches 
> into small self-contained pieces. please advice how to proceed.

I was meaning sending this patch along with others together as a patch series.
They are still distinct changes, self-contained in each patch and possible to
apply separately in order, but submitted together in order to make the review
more meaningful.

- Mark

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel