Re: [libav-devel] [PATCH] dsputil: Split off H.263 bits into their own H263DSPContext
On Fri, 8 Nov 2013, Diego Biurrun wrote: --- Now initializing h263dsp in the places where it gets used instead of doing a blanket initialization in mpegvideo.c. configure |9 ++-- libavcodec/Makefile |1 + libavcodec/dsputil.c | 79 -- libavcodec/dsputil.h |3 -- libavcodec/h263.c | 33 +++-- libavcodec/h263data.h |5 -- libavcodec/h263dec.c |1 + libavcodec/h263dsp.c | 108 + libavcodec/h263dsp.h | 34 + libavcodec/mpegvideo.h|3 +- libavcodec/mpegvideo_enc.c|1 + libavcodec/rv10.c |1 + libavcodec/x86/Makefile |4 +- libavcodec/x86/dsputil_init.c |8 --- libavcodec/x86/h263dsp_init.c | 39 +++ 15 files changed, 211 insertions(+), 118 deletions(-) create mode 100644 libavcodec/h263dsp.c create mode 100644 libavcodec/h263dsp.h create mode 100644 libavcodec/x86/h263dsp_init.c LGTM now, assuming it passes standalone compilation tests. // Martin ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
[libav-devel] [PATCH] dsputil: Split off H.263 bits into their own H263DSPContext
---
Now initializing h263dsp in the places where it gets used instead of
doing a blanket initialization in mpegvideo.c.
configure |9 ++--
libavcodec/Makefile |1 +
libavcodec/dsputil.c | 79 --
libavcodec/dsputil.h |3 --
libavcodec/h263.c | 33 +++--
libavcodec/h263data.h |5 --
libavcodec/h263dec.c |1 +
libavcodec/h263dsp.c | 108 +
libavcodec/h263dsp.h | 34 +
libavcodec/mpegvideo.h|3 +-
libavcodec/mpegvideo_enc.c|1 +
libavcodec/rv10.c |1 +
libavcodec/x86/Makefile |4 +-
libavcodec/x86/dsputil_init.c |8 ---
libavcodec/x86/h263dsp_init.c | 39 +++
15 files changed, 211 insertions(+), 118 deletions(-)
create mode 100644 libavcodec/h263dsp.c
create mode 100644 libavcodec/h263dsp.h
create mode 100644 libavcodec/x86/h263dsp_init.c
diff --git a/configure b/configure
index b83f9a3..664fe94 100755
--- a/configure
+++ b/configure
@@ -1384,6 +1384,7 @@ CONFIG_EXTRA="
gcrypt
golomb
gplv3
+h263dsp
h264chroma
h264dsp
h264pred
@@ -1598,8 +1599,8 @@ g2m_decoder_deps="zlib"
g2m_decoder_select="dsputil"
h261_decoder_select="error_resilience mpegvideo"
h261_encoder_select="aandcttables mpegvideoenc"
-h263_decoder_select="error_resilience h263_parser mpegvideo"
-h263_encoder_select="aandcttables mpegvideoenc"
+h263_decoder_select="error_resilience h263_parser h263dsp mpegvideo"
+h263_encoder_select="aandcttables h263dsp mpegvideoenc"
h263i_decoder_select="h263_decoder"
h263p_encoder_select="h263_encoder"
h264_decoder_select="golomb h264chroma h264dsp h264pred h264qpel videodsp"
@@ -1665,9 +1666,9 @@ qcelp_decoder_select="lsp"
qdm2_decoder_select="mdct rdft mpegaudiodsp"
ra_144_encoder_select="audio_frame_queue lpc"
ralf_decoder_select="golomb"
-rv10_decoder_select="error_resilience h263_decoder"
+rv10_decoder_select="error_resilience h263_decoder h263dsp"
rv10_encoder_select="h263_encoder"
-rv20_decoder_select="error_resilience h263_decoder"
+rv20_decoder_select="error_resilience h263_decoder h263dsp"
rv20_encoder_select="h263_encoder"
rv30_decoder_select="error_resilience golomb h264chroma h264pred h264qpel
mpegvideo videodsp"
rv40_decoder_select="error_resilience golomb h264chroma h264pred h264qpel
mpegvideo videodsp"
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 205359e..03d7459 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -41,6 +41,7 @@ FFT-OBJS-$(CONFIG_HARDCODED_TABLES)+= cos_tables.o
cos_fixed_tables.o
OBJS-$(CONFIG_FFT) += avfft.o fft_fixed.o fft_float.o \
$(FFT-OBJS-yes)
OBJS-$(CONFIG_GOLOMB) += golomb.o
+OBJS-$(CONFIG_H263DSP) += h263dsp.o
OBJS-$(CONFIG_H264CHROMA) += h264chroma.o
OBJS-$(CONFIG_H264DSP) += h264dsp.o h264idct.o
OBJS-$(CONFIG_H264PRED)+= h264pred.o
diff --git a/libavcodec/dsputil.c b/libavcodec/dsputil.c
index 6d93706..5839eb3 100644
--- a/libavcodec/dsputil.c
+++ b/libavcodec/dsputil.c
@@ -1409,80 +1409,6 @@ static void put_mspel8_mc22_c(uint8_t *dst, uint8_t
*src, ptrdiff_t stride)
wmv2_mspel8_v_lowpass(dst, halfH+8, stride, 8, 8);
}
-static void h263_v_loop_filter_c(uint8_t *src, int stride, int qscale){
-if(CONFIG_H263_DECODER || CONFIG_H263_ENCODER) {
-int x;
-const int strength= ff_h263_loop_filter_strength[qscale];
-
-for(x=0; x<8; x++){
-int d1, d2, ad1;
-int p0= src[x-2*stride];
-int p1= src[x-1*stride];
-int p2= src[x+0*stride];
-int p3= src[x+1*stride];
-int d = (p0 - p3 + 4*(p2 - p1)) / 8;
-
-if (d<-2*strength) d1= 0;
-else if(d<- strength) d1=-2*strength - d;
-else if(d< strength) d1= d;
-else if(d< 2*strength) d1= 2*strength - d;
-else d1= 0;
-
-p1 += d1;
-p2 -= d1;
-if(p1&256) p1= ~(p1>>31);
-if(p2&256) p2= ~(p2>>31);
-
-src[x-1*stride] = p1;
-src[x+0*stride] = p2;
-
-ad1= FFABS(d1)>>1;
-
-d2= av_clip((p0-p3)/4, -ad1, ad1);
-
-src[x-2*stride] = p0 - d2;
-src[x+ stride] = p3 + d2;
-}
-}
-}
-
-static void h263_h_loop_filter_c(uint8_t *src, int stride, int qscale){
-if(CONFIG_H263_DECODER || CONFIG_H263_ENCODER) {
-int y;
-const int strength= ff_h263_loop_filter_strength[qscale];
-
-for(y=0; y<8; y++){
-int d1, d2, ad1;
-int p0= src[y*stride-2];
-int p1= src[y*stride-1];
-int p2= src[y*stride+0];
-int p3= src[y*stride+1];
-int d = (p0 - p3 + 4*(p2 - p1)) / 8;
-
-if (d<-2*strength) d1= 0;
-else if(d<- strength) d1=-2*strength - d;
-else if(d< strength) d1= d;
-
Re: [libav-devel] [PATCH] avienc: drop the vfr flag.
On 07/11/13 22:06, Anton Khirnov wrote:
> AVI does not really support vfr properly, only by padding with null
> packets.
> ---
> libavformat/avienc.c |1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/libavformat/avienc.c b/libavformat/avienc.c
> index 66339af..0b1d578 100644
> --- a/libavformat/avienc.c
> +++ b/libavformat/avienc.c
> @@ -632,5 +632,4 @@ AVOutputFormat ff_avi_muxer = {
> .codec_tag = (const AVCodecTag* const []){
> ff_codec_bmp_tags, ff_codec_wav_tags, 0
> },
> -.flags = AVFMT_VARIABLE_FPS,
> };
Ok.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
[libav-devel] [PATCH] avienc: drop the vfr flag.
AVI does not really support vfr properly, only by padding with null
packets.
---
libavformat/avienc.c |1 -
1 file changed, 1 deletion(-)
diff --git a/libavformat/avienc.c b/libavformat/avienc.c
index 66339af..0b1d578 100644
--- a/libavformat/avienc.c
+++ b/libavformat/avienc.c
@@ -632,5 +632,4 @@ AVOutputFormat ff_avi_muxer = {
.codec_tag = (const AVCodecTag* const []){
ff_codec_bmp_tags, ff_codec_wav_tags, 0
},
-.flags = AVFMT_VARIABLE_FPS,
};
--
1.7.10.4
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 2/2] h263dsp: K&R formatting cosmetics
On Thu, 7 Nov 2013, Diego Biurrun wrote:
On Thu, Nov 07, 2013 at 03:29:24PM +0200, Martin Storsjö wrote:
On Mon, 4 Nov 2013, Diego Biurrun wrote:
--- a/libavcodec/h263dsp.c
+++ b/libavcodec/h263dsp.c
@@ -23,78 +23,94 @@
#include "config.h"
#include "h263dsp.h"
-const uint8_t ff_h263_loop_filter_strength[32]={
-// 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24
25 26 27 28 29 30 31
-0, 1, 1, 2, 2, 3, 3, 4, 4, 4, 5, 5, 6, 6, 7, 7, 7, 8, 8, 8, 9, 9,
9,10,10,10,11,11,11,12,12,12
This also removes some commented out code - that's maybe a part of a
general cosmetic cleanup, but when the topic is "K&R formatting
cosmetics" I would expect it to be a pure reindentation patch,
because that's the only thing that K&R says anything about at all.
Please at least mention this in the commit message in some way.
(Ideally please also dig through the git history to figure out what
these commented out coefficientes are and why they were kept to
begin with and mention that in the commit message.)
It's been there since the beginning. IMO these are not coefficients,
just a numbering of the array elements from 0 - 31.
Right, that makes sense.
If you look at h263data.h, you will see more instances, none were
changed since they were introduced. Would you be happy with either of
"Also remove some silly comments."
"Also remove array element numbering comments."
added to the log message?
The latter sounds good to me.
-static void h263_v_loop_filter_c(uint8_t *src, int stride, int qscale){
+static void h263_v_loop_filter_c(uint8_t *src, int stride, int qscale)
+{
int x;
-const int strength= ff_h263_loop_filter_strength[qscale];
+const int strength = ff_h263_loop_filter_strength[qscale];
-for(x=0; x<8; x++){
+for (x = 0; x < 8; x++) {
int d1, d2, ad1;
-int p0= src[x-2*stride];
-int p1= src[x-1*stride];
-int p2= src[x+0*stride];
-int p3= src[x+1*stride];
-int d = (p0 - p3 + 4*(p2 - p1)) / 8;
-
-if (d<-2*strength) d1= 0;
-else if(d<- strength) d1=-2*strength - d;
-else if(d< strength) d1= d;
-else if(d< 2*strength) d1= 2*strength - d;
-else d1= 0;
+int p0 = src[x - 2 * stride];
+int p1 = src[x - 1 * stride];
+int p2 = src[x + 0 * stride];
+int p3 = src[x + 1 * stride];
+int d = (p0 - p3 + 4 * (p2 - p1)) / 8;
+
+if (d < -2 * strength)
+d1 = 0;
+else if (d < -strength)
+d1 = -2 * strength - d;
+else if (d < strength)
+d1 = d;
+else if (d < 2 * strength)
+d1 = 2 * strength - d;
+else
+d1 = 0;
Some might argue that this was more readable in the previous form,
but I'm ok with it in this form as well, and this obviously matches
the general formatting style closer. (Keeping things on one line is
ok with me in the odd cases where it significantly helps readability
- in this case I'm not sure how significantly it helps.)
IMO this hardly helps here, so I decided against idiosyncratic formatting.
Ok, sounds good to me.
// Martin
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 2/2] h263dsp: K&R formatting cosmetics
On Thu, Nov 07, 2013 at 03:29:24PM +0200, Martin Storsjö wrote:
> On Mon, 4 Nov 2013, Diego Biurrun wrote:
> >--- a/libavcodec/h263dsp.c
> >+++ b/libavcodec/h263dsp.c
> >@@ -23,78 +23,94 @@
> >#include "config.h"
> >#include "h263dsp.h"
> >
> >-const uint8_t ff_h263_loop_filter_strength[32]={
> >-// 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23
> >24 25 26 27 28 29 30 31
> >-0, 1, 1, 2, 2, 3, 3, 4, 4, 4, 5, 5, 6, 6, 7, 7, 7, 8, 8, 8, 9, 9,
> >9,10,10,10,11,11,11,12,12,12
>
> This also removes some commented out code - that's maybe a part of a
> general cosmetic cleanup, but when the topic is "K&R formatting
> cosmetics" I would expect it to be a pure reindentation patch,
> because that's the only thing that K&R says anything about at all.
>
> Please at least mention this in the commit message in some way.
> (Ideally please also dig through the git history to figure out what
> these commented out coefficientes are and why they were kept to
> begin with and mention that in the commit message.)
It's been there since the beginning. IMO these are not coefficients,
just a numbering of the array elements from 0 - 31.
If you look at h263data.h, you will see more instances, none were
changed since they were introduced. Would you be happy with either of
"Also remove some silly comments."
"Also remove array element numbering comments."
added to the log message?
> >-static void h263_v_loop_filter_c(uint8_t *src, int stride, int qscale){
> >+static void h263_v_loop_filter_c(uint8_t *src, int stride, int qscale)
> >+{
> >int x;
> >-const int strength= ff_h263_loop_filter_strength[qscale];
> >+const int strength = ff_h263_loop_filter_strength[qscale];
> >
> >-for(x=0; x<8; x++){
> >+for (x = 0; x < 8; x++) {
> >int d1, d2, ad1;
> >-int p0= src[x-2*stride];
> >-int p1= src[x-1*stride];
> >-int p2= src[x+0*stride];
> >-int p3= src[x+1*stride];
> >-int d = (p0 - p3 + 4*(p2 - p1)) / 8;
> >-
> >-if (d<-2*strength) d1= 0;
> >-else if(d<- strength) d1=-2*strength - d;
> >-else if(d< strength) d1= d;
> >-else if(d< 2*strength) d1= 2*strength - d;
> >-else d1= 0;
> >+int p0 = src[x - 2 * stride];
> >+int p1 = src[x - 1 * stride];
> >+int p2 = src[x + 0 * stride];
> >+int p3 = src[x + 1 * stride];
> >+int d = (p0 - p3 + 4 * (p2 - p1)) / 8;
> >+
> >+if (d < -2 * strength)
> >+d1 = 0;
> >+else if (d < -strength)
> >+d1 = -2 * strength - d;
> >+else if (d < strength)
> >+d1 = d;
> >+else if (d < 2 * strength)
> >+d1 = 2 * strength - d;
> >+else
> >+d1 = 0;
>
> Some might argue that this was more readable in the previous form,
> but I'm ok with it in this form as well, and this obviously matches
> the general formatting style closer. (Keeping things on one line is
> ok with me in the odd cases where it significantly helps readability
> - in this case I'm not sure how significantly it helps.)
IMO this hardly helps here, so I decided against idiosyncratic formatting.
Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
[libav-devel] [PATCH 1/2] configure: Set up most temp files before handling --toolchain
The temp executable file isn't created along with the others,
since the file name depends on the executable extension, which
depends on target_os, and the --toolchain option can change
the default value of target_os.
---
The previous version of this patch didn't set pkg-config properly
and would also fail to set target_os properly in some cases.
---
configure | 61 +++--
1 file changed, 31 insertions(+), 30 deletions(-)
diff --git a/configure b/configure
index b83f9a3..86aac19 100755
--- a/configure
+++ b/configure
@@ -2248,6 +2248,37 @@ strip="${cross_prefix}${strip}"
sysinclude_default="${sysroot}/usr/include"
+# set temporary file name
+: ${TMPDIR:=$TEMPDIR}
+: ${TMPDIR:=$TMP}
+: ${TMPDIR:=/tmp}
+
+if ! check_cmd mktemp -u XX; then
+# simple replacement for missing mktemp
+# NOT SAFE FOR GENERAL USE
+mktemp(){
+echo "${2%%XXX*}.${HOSTNAME}.${UID}.$$"
+}
+fi
+
+tmpfile(){
+tmp=$(mktemp -u "${TMPDIR}/ffconf.")$2 &&
+(set -C; exec > $tmp) 2>/dev/null ||
+die "Unable to create temporary file in $TMPDIR."
+append TMPFILES $tmp
+eval $1=$tmp
+}
+
+trap 'rm -f -- $TMPFILES' EXIT
+
+tmpfile TMPASM .asm
+tmpfile TMPC .c
+tmpfile TMPH .h
+tmpfile TMPO .o
+tmpfile TMPS .S
+tmpfile TMPSH .sh
+tmpfile TMPV .ver
+
case "$toolchain" in
clang-asan)
cc_default="clang"
@@ -2322,37 +2353,7 @@ exesuf() {
EXESUF=$(exesuf $target_os)
HOSTEXESUF=$(exesuf $host_os)
-# set temporary file name
-: ${TMPDIR:=$TEMPDIR}
-: ${TMPDIR:=$TMP}
-: ${TMPDIR:=/tmp}
-
-if ! check_cmd mktemp -u XX; then
-# simple replacement for missing mktemp
-# NOT SAFE FOR GENERAL USE
-mktemp(){
-echo "${2%%XXX*}.${HOSTNAME}.${UID}.$$"
-}
-fi
-
-tmpfile(){
-tmp=$(mktemp -u "${TMPDIR}/ffconf.")$2 &&
-(set -C; exec > $tmp) 2>/dev/null ||
-die "Unable to create temporary file in $TMPDIR."
-append TMPFILES $tmp
-eval $1=$tmp
-}
-
-trap 'rm -f -- $TMPFILES' EXIT
-
-tmpfile TMPASM .asm
-tmpfile TMPC .c
tmpfile TMPE $EXESUF
-tmpfile TMPH .h
-tmpfile TMPO .o
-tmpfile TMPS .S
-tmpfile TMPSH .sh
-tmpfile TMPV .ver
unset -f mktemp
--
1.7.9.4
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 2/2] h263dsp: K&R formatting cosmetics
On Mon, 4 Nov 2013, Diego Biurrun wrote:
---
libavcodec/h263dsp.c | 110 +-
1 file changed, 63 insertions(+), 47 deletions(-)
diff --git a/libavcodec/h263dsp.c b/libavcodec/h263dsp.c
index 1166b93..63d0972 100644
--- a/libavcodec/h263dsp.c
+++ b/libavcodec/h263dsp.c
@@ -23,78 +23,94 @@
#include "config.h"
#include "h263dsp.h"
-const uint8_t ff_h263_loop_filter_strength[32]={
-// 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24
25 26 27 28 29 30 31
-0, 1, 1, 2, 2, 3, 3, 4, 4, 4, 5, 5, 6, 6, 7, 7, 7, 8, 8, 8, 9, 9,
9,10,10,10,11,11,11,12,12,12
This also removes some commented out code - that's maybe a part of a
general cosmetic cleanup, but when the topic is "K&R formatting cosmetics"
I would expect it to be a pure reindentation patch, because that's the
only thing that K&R says anything about at all.
Please at least mention this in the commit message in some way. (Ideally
please also dig through the git history to figure out what these commented
out coefficientes are and why they were kept to begin with and mention
that in the commit message.)
+const uint8_t ff_h263_loop_filter_strength[32] = {
+0, 1, 1, 2, 2, 3, 3, 4, 4, 4, 5, 5, 6, 6, 7, 7,
+7, 8, 8, 8, 9, 9, 9, 10, 10, 10, 11, 11, 11, 12, 12, 12
};
-static void h263_v_loop_filter_c(uint8_t *src, int stride, int qscale){
+static void h263_v_loop_filter_c(uint8_t *src, int stride, int qscale)
+{
int x;
-const int strength= ff_h263_loop_filter_strength[qscale];
+const int strength = ff_h263_loop_filter_strength[qscale];
-for(x=0; x<8; x++){
+for (x = 0; x < 8; x++) {
int d1, d2, ad1;
-int p0= src[x-2*stride];
-int p1= src[x-1*stride];
-int p2= src[x+0*stride];
-int p3= src[x+1*stride];
-int d = (p0 - p3 + 4*(p2 - p1)) / 8;
-
-if (d<-2*strength) d1= 0;
-else if(d<- strength) d1=-2*strength - d;
-else if(d< strength) d1= d;
-else if(d< 2*strength) d1= 2*strength - d;
-else d1= 0;
+int p0 = src[x - 2 * stride];
+int p1 = src[x - 1 * stride];
+int p2 = src[x + 0 * stride];
+int p3 = src[x + 1 * stride];
+int d = (p0 - p3 + 4 * (p2 - p1)) / 8;
+
+if (d < -2 * strength)
+d1 = 0;
+else if (d < -strength)
+d1 = -2 * strength - d;
+else if (d < strength)
+d1 = d;
+else if (d < 2 * strength)
+d1 = 2 * strength - d;
+else
+d1 = 0;
Some might argue that this was more readable in the previous form, but I'm
ok with it in this form as well, and this obviously matches the general
formatting style closer. (Keeping things on one line is ok with me in the
odd cases where it significantly helps readability - in this case I'm not
sure how significantly it helps.)
// Martin
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH] [v2] dsputil: Split off H.263 bits into their own H263DSPContext
On Tue, 5 Nov 2013, Diego Biurrun wrote: --- Adjusted the bits in configure so that mpegvideo now automatically selects h263dsp. Given that the h263dsp init is called unconditionally from mpegvideo.c, this is the correct solution. Given that h263dsp init is called unconditionally from mpegvideo.c, this is the correct solution yes. The question the other way around is, does this have to be initialized unconditionally from within mpegvideo.c, when it could be initialized e.g. in ff_h263_decode_init (and something similar for the encoder as well)? That would allow untangling these functions from mpegvideo (even though the struct might be kept in the mpegvideo contexts), to avoid including them in some builds. What, if any, components are there that include mpegvideo but don't depend on the h263 decoder? configure |3 +- libavcodec/Makefile |1 + libavcodec/dsputil.c | 79 -- libavcodec/dsputil.h |3 -- libavcodec/h263.c | 33 +++-- libavcodec/h263data.h |5 -- libavcodec/h263dsp.c | 108 + libavcodec/h263dsp.h | 34 + libavcodec/mpegvideo.c|1 + libavcodec/mpegvideo.h|3 +- libavcodec/x86/Makefile |4 +- libavcodec/x86/dsputil_init.c |8 --- libavcodec/x86/h263dsp_init.c | 39 +++ 13 files changed, 206 insertions(+), 115 deletions(-) create mode 100644 libavcodec/h263dsp.c create mode 100644 libavcodec/h263dsp.h create mode 100644 libavcodec/x86/h263dsp_init.c The patch itself looks good, except for the place where it's initialized. // Martin ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] fate: Add FFV1 tests
Quoting Diego Biurrun :
From 9fde43d493e6ce3216dd4ff779a8440fb647d8db Mon Sep 17 00:00:00 2001
From: Peter B.
Date: Wed, 6 Nov 2013 14:50:53 +0100
Subject: [PATCH] - Improved FFV1 fate test speed. - Added dependencies:
decoding requires encoding before - Added dependency: pass2
requires pass1 - Added dependency: vsynth1.yuv must be
generated - Added conditionals to work with multiple
targets (ffmpeg,avconv)
This list of changes belongs in an annotation, not in the log message.
I still prefer the original log message I suggested :)
I'm not really familiar with auto-generated patches.
The Subject is auto-generated by "git format-patch" and is basically
my commit message.
I did change the mail-subject to your suggested log-message, but
wasn't aware that you also meant the message inside the patch.
Thanks for clearing that up.
--- /dev/null
+++ b/tests/fate/ffv1.mak
@@ -0,0 +1,313 @@
+# This Makefile checks for $(CONFIG_...) variables being set, so we can
+# include/exclude tests accordingly:
+ifdef CONFIG_AVCONV
+FLAGS_FFV1_V3 = -strict experimental
+else
+FLAGS_FFV1_V3 =
+endif
trailing whitespace
Oops.
Overlooked. Sorry.
This ifdef is redundant, avconv is required to run these tests in the
first place.
Well, yes and no:
In order to make it easier for me to maintain this FFV1 fate-testset,
I've written the Makefile in a way that the same file will build the
tests properly for both: FFmpeg and Libav.
So, I've used the "CONFIG_AVCONV" and "CONFIG_FFMPEG" variables as
indicator for which target system the tests are being ran.
That also explains other references to FFmpeg in that Makefile.
Just limit the number of frames directly. Ideally by cutting the samples
to 4 frames.
Hm...
I actually wanted to use vsynth1.yuv "as-is", but actually it's a good idea:
It would also enable to generate other pix_fmt and size versions out
of the vsynth source.
Where would be a good location to store such intermediate files?
In tests/data/fate somewhere?
+FATE_FFV1_LEVEL1 = v1-defaults \
+ v1-gray \
+ v1-rgb32 \
+ v1-yuv410p \
+ v1-yuv411p \
+ v1-yuv420p \
+ v1-yuv422p \
+ v1-yuv444p \
+ v1-bgra \
+ v1-tff \
+ v1-bff
nit: You could align the '\'.
Like this?
+FATE_FFV1_LEVEL1 = v1-defaults \
+ v1-gray \
+ v1-rgb32\
+###
+# Decoding:
+###
+# YUV (8bit)
+fate-ffv1-dec-v1-defaults: ${CMD = framecrc -i
$(DEC_SRC)/ffv1-enc-v1-defaults.avi} fate-ffv1-enc-v1-defaults
This works as expected?
Actually yes. Why?
Also, use $() syntax instead of ${}.
Roger that.
Thought it might be nice to have some difference between Makefile
variable-access use and actual Makefile-command sets.
It's purely cosmetic though, so I'll change it to whatever your custom is.
+###
+# Encoding:
+###
Some of these options could be factored out into variables.
You mean making variables to for-loop over?
This is my first Makefile and my first contact with FATE tests, so I
tried to keep it as straightforward as possible.
+# Requires generating vsynth1.yuv as input source:
+$(FATE_FFV1-yes): tests/data/vsynth1.yuv
pointless comment
I agree and disagree.
When I started to understand "tests/vcodec.mak", I didn't consider
this line necessary for ffv1.mak.
Only to realize days later, that "vsynth1.yuv" generating is triggered
if executing vcodec.mak - and so the ffv1 tests here "Requires
generating vsynth1.yuv as input source".
If there were some more of these kind of "pointless comments", I'd
wouldn't have had to spend several days reverse-engineering the "what
is where and why?" of FATE testing.
I am technically skilled, but not too familiar with Makefiles or C-code.
Comments might make the life of non-C-guru-contributors way easier.
No ranting, just my personal experience.
diff --git a/tests/ref/fate/ffv1-dec-v1-bff b/tests/ref/fate/ffv1-dec-v1-bff
new file mode 100644
index 000..e69de29
diff --git a/tests/ref/fate/ffv1-dec-v1-bgra
b/tests/ref/fate/ffv1-dec-v1-bgra
new file mode 100644
index 000..e69de29
diff --git a/tests/ref/fate/ffv1-dec-v1-defaults
b/tests/ref/fate/ffv1-dec-v1-defaults
new file mode 100644
index 000..e69de29
Empty files?
Hm...
Will check.
Thanks for your feedback!
Will incorporate the changes as soon as possible.
Regards,
Pb
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] fate: Add FFV1 tests
On Wed, Nov 06, 2013 at 02:54:24PM +0100, Peter B. wrote:
> Here's the new, improved version of the patch. YAY!
>
> Changes:
> - Improved the speed by reducing the sample durations to 4 frames
> - Fixed the 2pass dependency (and started learning Makefile basics)
> - Added prerequisites: Decoding requires encoding to be executed first
> - Added check for build-target, so the Makefile works with ffmpeg + avconv.
> - Added prerequisite for vsynth1.yuv being generated
>
> Feedback welcome.
>
> From 9fde43d493e6ce3216dd4ff779a8440fb647d8db Mon Sep 17 00:00:00 2001
> From: Peter B.
> Date: Wed, 6 Nov 2013 14:50:53 +0100
> Subject: [PATCH] - Improved FFV1 fate test speed. - Added dependencies:
> decoding requires encoding before - Added dependency: pass2
> requires pass1 - Added dependency: vsynth1.yuv must be
> generated - Added conditionals to work with multiple
> targets (ffmpeg,avconv)
This list of changes belongs in an annotation, not in the log message.
I still prefer the original log message I suggested :)
> --- /dev/null
> +++ b/tests/fate/ffv1.mak
> @@ -0,0 +1,313 @@
> +# This Makefile checks for $(CONFIG_...) variables being set, so we can
> +# include/exclude tests accordingly:
> +ifdef CONFIG_AVCONV
> +FLAGS_FFV1_V3 = -strict experimental
> +else
> +FLAGS_FFV1_V3 =
> +endif
trailing whitespace
This ifdef is redundant, avconv is required to run these tests in the
first place.
> +DEC_SRC = $(TARGET_PATH)/tests/data/fate
> +
> +fate-ffv1-enc-%: CODEC = $(word 2, $(subst -, ,$(@)))
> +fate-ffv1-enc-%: FMT = avi
> +fate-ffv1-enc-%: SRC = tests/data/vsynth1.yuv
> +# Limit the duration of test videos to 4 frames at 25fps:
> +fate-ffv1-enc-%: DUR = 0:00:00.160
> +fate-ffv1-enc-%: CMD = enc_dec "rawvideo -s 352x288 -pix_fmt yuv420p
> $(RAWDECOPTS)" $(SRC) $(FMT) "-t $(DUR) -c $(CODEC) $(ENCOPTS)" rawvideo "-s
> 352x288 -pix_fmt yuv420p -vsync 0 $(DECOPTS)" -keep "$(DECINOPTS)"
Just limit the number of frames directly. Ideally by cutting the samples
to 4 frames.
> +FATE_FFV1_LEVEL1 = v1-defaults \
> + v1-gray \
> + v1-rgb32 \
> + v1-yuv410p \
> + v1-yuv411p \
> + v1-yuv420p \
> + v1-yuv422p \
> + v1-yuv444p \
> + v1-bgra \
> + v1-tff \
> + v1-bff
nit: You could align the '\'.
> +# Target-specific tests:
> +ifdef CONFIG_FFMPEG
No such config here.
> +###
> +# Decoding:
> +###
> +# YUV (8bit)
> +fate-ffv1-dec-v1-defaults: ${CMD = framecrc -i
> $(DEC_SRC)/ffv1-enc-v1-defaults.avi} fate-ffv1-enc-v1-defaults
This works as expected?
Also, use $() syntax instead of ${}.
> +###
> +# Encoding:
> +###
> +# - This also iterates through slice variations (4, 12, 24, 30).
> +#
> +fate-ffv1-enc-v3-defaults: ENCOPTS = -level 3 $(FLAGS_FFV1_V3)
> +# YUV (8bit)
> +# - This also iterates through all coder/context combinations.
> +fate-ffv1-enc-v3-yuv410p:ENCOPTS = -level 3 -g 1 -coder 0 -context 0
> -slices 4 -slicecrc 0 -pix_fmt yuv410p $(FLAGS_FFV1_V3)
> +fate-ffv1-enc-v3-yuv420p:ENCOPTS = -level 3 -g 1 -coder 0 -context 1
> -slices 12 -slicecrc 0 -pix_fmt yuv420p $(FLAGS_FFV1_V3)
> +fate-ffv1-enc-v3-yuv422p:ENCOPTS = -level 3 -g 1 -coder 1 -context 0
> -slices 24 -slicecrc 0 -pix_fmt yuv422p $(FLAGS_FFV1_V3)
> +fate-ffv1-enc-v3-yuv444p:ENCOPTS = -level 3 -g 1 -coder 1 -context 1
> -slices 30 -slicecrc 0 -pix_fmt yuv444p $(FLAGS_FFV1_V3)
> +# YUV (9bit)
> +fate-ffv1-enc-v3-yuv420p9: ENCOPTS = -level 3 -g 1 -coder -1 -context
> 1 -slices 24 -slicecrc 0 -pix_fmt yuv420p9 $(FLAGS_FFV1_V3)
> +fate-ffv1-enc-v3-yuv422p9: ENCOPTS = -level 3 -g 1 -coder 2 -context 0
> -slices 30 -slicecrc 0 -pix_fmt yuv422p9 $(FLAGS_FFV1_V3)
> +fate-ffv1-enc-v3-yuv444p9: ENCOPTS = -level 3 -g 1 -coder 1 -context 1
> -slices 4 -slicecrc 0 -pix_fmt yuv444p9 $(FLAGS_FFV1_V3)
> +# YUV (10bit)
> +fate-ffv1-enc-v3-yuv420p10: ENCOPTS = -level 3 -g 1 -coder 1 -context 1
> -slices 30 -slicecrc 0 -pix_fmt yuv420p10 $(FLAGS_FFV1_V3)
> +fate-ffv1-enc-v3-yuv422p10: ENCOPTS = -level 3 -g 1 -coder 1 -context 0
> -slices 4 -slicecrc 0 -pix_fmt yuv422p10 $(FLAGS_FFV1_V3)
> +fate-ffv1-enc-v3-yuv444p10: ENCOPTS = -level 3 -g 1 -coder 1 -context 1
> -slices 12 -slicecrc 0 -pix_fmt yuv444p10 $(FLAGS_FFV1_V3)
> +# YUV (16bit)
> +fate-ffv1-enc-v3-yuv420p16: ENCOPTS = -level 3 -g 1 -coder 1 -context 1
> -slices 4 -slicecrc 0 -pix_fmt yuv420p16 $(FLAGS_FFV1_V3)
> +fate-ffv1-enc-v3-yuv422p16: ENCOPTS = -level 3 -g 1 -coder 1 -context 0
> -slices 12 -slicecrc 0 -pix_fmt yuv422p16 $(FLAGS_FFV1_V3)
> +fate-ffv1-enc-v3-yuv444p16: E
Re: [libav-devel] [PATCH] png: add a standalone parser
On Mon, 4 Nov 2013 17:05:52 +0100, Vittorio Giovara wrote: > From: Peter Holik > > --- > Addressed Kostya's comments and added some minor comments. > Vittorio > > libavcodec/Makefile |1 + > libavcodec/allcodecs.c |1 + > libavcodec/png_parser.c | 124 > +++ > 3 files changed, 126 insertions(+) > create mode 100644 libavcodec/png_parser.c > > diff --git a/libavcodec/Makefile b/libavcodec/Makefile > index 205359e..0b7eba6 100644 > --- a/libavcodec/Makefile > +++ b/libavcodec/Makefile > @@ -648,6 +648,7 @@ OBJS-$(CONFIG_MPEGAUDIO_PARSER)+= > mpegaudio_parser.o \ >mpegaudiodecheader.o > mpegaudiodata.o > OBJS-$(CONFIG_MPEGVIDEO_PARSER)+= mpegvideo_parser.o\ >mpeg12.o mpeg12data.o > +OBJS-$(CONFIG_PNG_PARSER) += png_parser.o > OBJS-$(CONFIG_PNM_PARSER) += pnm_parser.o pnm.o > OBJS-$(CONFIG_RV30_PARSER) += rv34_parser.o > OBJS-$(CONFIG_RV40_PARSER) += rv34_parser.o > diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c > index 6172466..bc81c49 100644 > --- a/libavcodec/allcodecs.c > +++ b/libavcodec/allcodecs.c > @@ -460,6 +460,7 @@ void avcodec_register_all(void) > REGISTER_PARSER(MPEG4VIDEO, mpeg4video); > REGISTER_PARSER(MPEGAUDIO, mpegaudio); > REGISTER_PARSER(MPEGVIDEO, mpegvideo); > +REGISTER_PARSER(PNG,png); > REGISTER_PARSER(PNM,pnm); > REGISTER_PARSER(RV30, rv30); > REGISTER_PARSER(RV40, rv40); > diff --git a/libavcodec/png_parser.c b/libavcodec/png_parser.c > new file mode 100644 > index 000..00cbb82 > --- /dev/null > +++ b/libavcodec/png_parser.c > @@ -0,0 +1,124 @@ > +/* > + * PNG parser > + * Copyright (c) 2009 Peter Holik > + * > + * This file is part of Libav. > + * > + * Libav is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * Libav is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with Libav; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 > USA > + */ > + > +/** > + * @file > + * PNG parser > + */ > + > +#include "libavutil/intreadwrite.h" > +#include "libavutil/common.h" > + > +#include "parser.h" > + > +#define PNG_SIGNATURE UINT64_C(0x89504e470d0a1a0a) > +#define MNG_SIGNATURE UINT64_C(0x8a4d4e470d0a1a0a) This needs stdint.h I think. Also a bump+Changelog entry is in order. Otherwise should be fine if Костя thinks it's ok. -- Anton Khirnov ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
