Re: [libav-devel] [PATCH 1/3] ffv1: split decoder and encoder

2012-10-20 Thread Kostya Shishkov
On Fri, Oct 19, 2012 at 09:55:40PM +0200, Luca Barbato wrote:
> ---
>  libavcodec/Makefile  |4 +-
>  libavcodec/ffv1.c| 1612 
> +-
>  libavcodec/ffv1.h|  184 ++
>  libavcodec/ffv1dec.c |  636 
>  libavcodec/ffv1enc.c |  871 +++
>  5 files changed, 1705 insertions(+), 1602 deletions(-)
>  create mode 100644 libavcodec/ffv1.h
>  create mode 100644 libavcodec/ffv1dec.c
>  create mode 100644 libavcodec/ffv1enc.c

LGTM except that it still has
#endif /* AVCODEC_FFV1 */
instead of
#endif /* AVCODEC_FFV1_H */

Please fix that before pushing.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


Re: [libav-devel] [PATCH 1/3] ffv1: split decoder and encoder

2012-10-19 Thread Måns Rullgård
Luca Barbato  writes:

> On 10/19/2012 03:29 PM, Kostya Shishkov wrote:
>>> -static const int8_t quant5_10bit[256] = {
>>> > +const int8_t quant5_10bit[256] = {
>> don't we have to prefix such names?
>
> We hide everything not matching av_/avpriv_

That only works in shared libs.  Add a prefix.

-- 
Måns Rullgård
m...@mansr.com
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


Re: [libav-devel] [PATCH 1/3] ffv1: split decoder and encoder

2012-10-19 Thread Luca Barbato
On 10/19/2012 03:29 PM, Kostya Shishkov wrote:
>> -static const int8_t quant5_10bit[256] = {
>> > +const int8_t quant5_10bit[256] = {
> don't we have to prefix such names?

We hide everything not matching av_/avpriv_

lu
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


Re: [libav-devel] [PATCH 1/3] ffv1: split decoder and encoder

2012-10-19 Thread Kostya Shishkov
On Fri, Oct 19, 2012 at 12:16:33PM +0200, Luca Barbato wrote:
> ---
>  libavcodec/Makefile  |4 +-
>  libavcodec/ffv1.c| 1612 
> +-
>  libavcodec/ffv1.h|  184 ++
>  libavcodec/ffv1dec.c |  636 
>  libavcodec/ffv1enc.c |  871 +++
>  5 files changed, 1705 insertions(+), 1602 deletions(-)
>  create mode 100644 libavcodec/ffv1.h
>  create mode 100644 libavcodec/ffv1dec.c
>  create mode 100644 libavcodec/ffv1enc.c
> 
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index a63e2ba..05054ca 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -161,8 +161,8 @@ OBJS-$(CONFIG_EIGHTBPS_DECODER)+= 8bps.o
>  OBJS-$(CONFIG_EIGHTSVX_EXP_DECODER)+= 8svx.o
>  OBJS-$(CONFIG_EIGHTSVX_FIB_DECODER)+= 8svx.o
>  OBJS-$(CONFIG_ESCAPE124_DECODER)   += escape124.o
> -OBJS-$(CONFIG_FFV1_DECODER)+= ffv1.o
> -OBJS-$(CONFIG_FFV1_ENCODER)+= ffv1.o
> +OBJS-$(CONFIG_FFV1_DECODER)+= ffv1.o ffv1dec.o
> +OBJS-$(CONFIG_FFV1_ENCODER)+= ffv1.o ffv1enc.o

nit: I'd swap object files order - it's logical that codec-specific name goes
first and shared data follows.

>  OBJS-$(CONFIG_FFVHUFF_DECODER) += huffyuv.o
>  OBJS-$(CONFIG_FFVHUFF_ENCODER) += huffyuv.o
>  OBJS-$(CONFIG_FLAC_DECODER)+= flacdec.o flacdata.o flac.o 
> flacdsp.o
> diff --git a/libavcodec/ffv1.c b/libavcodec/ffv1.c
> index f290b94..7ef1056 100644
> --- a/libavcodec/ffv1.c
> +++ b/libavcodec/ffv1.c
> @@ -33,16 +33,9 @@
>  #include "rangecoder.h"
>  #include "golomb.h"
>  #include "mathops.h"
> +#include "ffv1.h"
>  
> -#define MAX_PLANES 4
> -#define CONTEXT_SIZE 32
> -
> -#define MAX_QUANT_TABLES 8
> -#define MAX_CONTEXT_INPUTS 5
> -
> -extern const uint8_t ff_log2_run[41];
> -
> -static const int8_t quant5_10bit[256] = {
> +const int8_t quant5_10bit[256] = {

don't we have to prefix such names?

>   0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  1,  1,  1,  1,  1,
>   1,  1,  1,  1,  1,  1,  1,  1,  1,  1,  1,  1,  1,  1,  1,  1,
>   1,  1,  1,  1,  1,  1,  1,  1,  1,  1,  1,  1,  1,  1,  1,  1,
[...]
> diff --git a/libavcodec/ffv1.h b/libavcodec/ffv1.h
> new file mode 100644
> index 000..0b21d95
> --- /dev/null
> +++ b/libavcodec/ffv1.h
[...]
> +
> +#endif /* AVCODEC_FFV1 */

AVCODEC_FFV1_H

the rest LGTM
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel