| Message ID | 1477204842-22572-1-git-send-email-thomastdt@googlemail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, Oct 22, 2016 at 11:40:42PM -0700, Thomas Turner wrote: > Function(s) Tested: av_packet_clone(). > > This test checks if av_packet_clone() can successfully make a copy of an AVPacket. > Compares all data members in AVPacket EXCEPT for "buf" because "buf" is initialized > to NIL in the original AVPacket [to be cloned]. > > This test also prints out the all the contents of the original and cloned AVPackets. > > Signed-off-by: Thomas Turner <thomastdt@googlemail.com> > --- > libavcodec/Makefile | 3 +- > libavcodec/tests/avpacket.c | 303 ++++++++++++++++++++++++++++++++++++++++++++ > tests/fate/libavcodec.mak | 5 + > 3 files changed, 310 insertions(+), 1 deletion(-) > create mode 100644 libavcodec/tests/avpacket.c > > diff --git a/libavcodec/Makefile b/libavcodec/Makefile > index f1d5bf1..46e3af7 100644 > --- a/libavcodec/Makefile > +++ b/libavcodec/Makefile > @@ -1019,7 +1019,8 @@ SKIPHEADERS-$(CONFIG_VDA) += vda.h vda_vt_internal.h > SKIPHEADERS-$(CONFIG_VDPAU) += vdpau.h vdpau_internal.h > SKIPHEADERS-$(CONFIG_VIDEOTOOLBOX) += videotoolbox.h vda_vt_internal.h > > -TESTPROGS = imgconvert \ > +TESTPROGS = avpacket \ > + imgconvert \ > jpeg2000dwt \ > mathops \ > options \ > diff --git a/libavcodec/tests/avpacket.c b/libavcodec/tests/avpacket.c > new file mode 100644 > index 0000000..752cf3c > --- /dev/null > +++ b/libavcodec/tests/avpacket.c > @@ -0,0 +1,303 @@ > +/* > + * This file is part of FFmpeg. > + * > + * FFmpeg 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. > + * > + * FFmpeg 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 FFmpeg; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > + */ > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <inttypes.h> > +#include <string.h> > +#include "libavcodec/avcodec.h" > +#include "libavutil/error.h" > + > + > + > + > +#define AVPACKET_FIELDS \ > + X(static AVBufferRef*, buf, "%p") \ > + X(static int64_t, pts, "%" PRId64) \ > + X(static int64_t, dts, "%" PRId64) \ > + X(static uint8_t*, data, "%p") \ > + X(static int, size, "%d") \ > + X(static int, stream_index, "%d") \ > + X(static int, flags, "%d") \ > + X(static AVPacketSideData*, side_data, "%p") \ > + X(static int, side_data_elems, "%d") \ > + X(static int64_t, duration, "%" PRId64) \ > + X(static int64_t, pos, "%" PRId64) > + > +#define AVBUFFERREF_FIELDS \ > + Y(static AVBuffer*, buffer, "%p") \ > + Y(static uint8_t*, data, "%p") \ > + Y(static int, size, "%d") > + > +#define AVPACKETSIDEDATA_FIELDS \ > + Z(static uint8_t*, data, "%p") \ > + Z(static int, size, "%d") \ > + Z(static enum AVPacketSideDataType, type, "%d") > + > + > + > + > +#define AV_MESSAGE_1(format, name, p1, p2) \ > + do { \ > + const char* str = "cloned variable \"%s\" equals " format \ > + " but should be " format "\n"; \ > + av_log(NULL, AV_LOG_ERROR, str, #name, p2->name, p1->name ); \ > + } while (0) > + > +#define AV_MESSAGE_2(name, p1, p2) \ > + do { \ > + const char* str = "contents of cloned variable " \ > + "%s equals \"%.*s\"\nbut should be \"%.*s\"\n"; \ > + av_log(NULL, AV_LOG_ERROR, str, #name, p1->size, \ > + p2->data, p1->size, p1->data); \ > + } while(0) > + > +#define AV_MESSAGE_3(name) \ > + do { \ > + const char* str = "error! did not attempt to compare " \ > + "contents of \"%s\" variable\n"; \ > + av_log(NULL, AV_LOG_ERROR, str, #name); \ > + } while(0) using these macros makes the code harder to read and maintain direct use of av_log() would be better also numbering functions like AV_MESSAGE_# is confusing, each should if used have a descriptive name > + > +#define AV_MESSAGE_4(string) \ > + do { av_log(NULL, AV_LOG_INFO, "%s", string); } while(0) a plain av_log() is clearer no need for a macro if you dont like the extra arguments for av_log() you can use printf() in code in tests/* > + > + > + > + > +#define COMPARE(a, b) ( ((a) == (b)) ? 1 : 0 ) this should not be needed a == b and a != b directly should work fine [...]
On Sat, 22 Oct 2016 23:40:42 -0700 Thomas Turner <thomastdt@googlemail.com> wrote: > Function(s) Tested: av_packet_clone(). > > This test checks if av_packet_clone() can successfully make a copy of an AVPacket. > Compares all data members in AVPacket EXCEPT for "buf" because "buf" is initialized > to NIL in the original AVPacket [to be cloned]. > > This test also prints out the all the contents of the original and cloned AVPackets. > > Signed-off-by: Thomas Turner <thomastdt@googlemail.com> > --- Am I the only one who thinks this test is overly complex and worthless? (Checks if each field got copied... come on, that's not useful. And would probably bitrot quickly as nobody would add new fields to the test anyway.)
On Wed, Oct 26, 2016 at 02:00:46PM +0200, wm4 wrote: > On Sat, 22 Oct 2016 23:40:42 -0700 > Thomas Turner <thomastdt@googlemail.com> wrote: > > > Function(s) Tested: av_packet_clone(). > > > > This test checks if av_packet_clone() can successfully make a copy of an AVPacket. > > Compares all data members in AVPacket EXCEPT for "buf" because "buf" is initialized > > to NIL in the original AVPacket [to be cloned]. > > > > This test also prints out the all the contents of the original and cloned AVPackets. > > > > Signed-off-by: Thomas Turner <thomastdt@googlemail.com> > > --- > > Am I the only one who thinks this test is overly complex and worthless? > (Checks if each field got copied... come on, that's not useful. And > would probably bitrot quickly as nobody would add new fields to the > test anyway.) what do you suggest, how to test it ? or should we deprecate av_packet_clone() nothing uses it, so the total lack of tests and uses means noone would notice (quickly) if it broke ever [...]
On Wed, 26 Oct 2016 14:56:01 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Wed, Oct 26, 2016 at 02:00:46PM +0200, wm4 wrote: > > On Sat, 22 Oct 2016 23:40:42 -0700 > > Thomas Turner <thomastdt@googlemail.com> wrote: > > > > > Function(s) Tested: av_packet_clone(). > > > > > > This test checks if av_packet_clone() can successfully make a copy of an AVPacket. > > > Compares all data members in AVPacket EXCEPT for "buf" because "buf" is initialized > > > to NIL in the original AVPacket [to be cloned]. > > > > > > This test also prints out the all the contents of the original and cloned AVPackets. > > > > > > Signed-off-by: Thomas Turner <thomastdt@googlemail.com> > > > --- > > > > Am I the only one who thinks this test is overly complex and worthless? > > (Checks if each field got copied... come on, that's not useful. And > > would probably bitrot quickly as nobody would add new fields to the > > test anyway.) > > what do you suggest, how to test it ? > or should we deprecate av_packet_clone() > nothing uses it, so the total lack of tests and uses means noone > would notice (quickly) if it broke ever > > [...] I don't think it's the choice between either adding a test for av_packet_clone or deprecating it? Also, av_packet_clone is 100% implemented using public functions and doesn't access any AVPacket fields itself.
On Wed, Oct 26, 2016 at 03:03:07PM +0200, wm4 wrote: > On Wed, 26 Oct 2016 14:56:01 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Wed, Oct 26, 2016 at 02:00:46PM +0200, wm4 wrote: > > > On Sat, 22 Oct 2016 23:40:42 -0700 > > > Thomas Turner <thomastdt@googlemail.com> wrote: > > > > > > > Function(s) Tested: av_packet_clone(). > > > > > > > > This test checks if av_packet_clone() can successfully make a copy of an AVPacket. > > > > Compares all data members in AVPacket EXCEPT for "buf" because "buf" is initialized > > > > to NIL in the original AVPacket [to be cloned]. > > > > > > > > This test also prints out the all the contents of the original and cloned AVPackets. > > > > > > > > Signed-off-by: Thomas Turner <thomastdt@googlemail.com> > > > > --- > > > > > > Am I the only one who thinks this test is overly complex and worthless? > > > (Checks if each field got copied... come on, that's not useful. And > > > would probably bitrot quickly as nobody would add new fields to the > > > test anyway.) > > > > what do you suggest, how to test it ? > > or should we deprecate av_packet_clone() > > nothing uses it, so the total lack of tests and uses means noone > > would notice (quickly) if it broke ever > > > > [...] > > I don't think it's the choice between either adding a test > for av_packet_clone or deprecating it? Its desirable to have code tested to identify problems, code thats not tested at all bit rots eventually. The issue is propotional to the complexity of the code and how activly it changes so the risk is low with av_packet_clone() but still > > Also, av_packet_clone is 100% implemented using public functions and > doesn't access any AVPacket fields itself. yes but these public functions themselfs arent tested for completness either. iam not arguing for a specific way of testing this, but that some clean test would be desirable [...]
diff --git a/libavcodec/Makefile b/libavcodec/Makefile index f1d5bf1..46e3af7 100644 --- a/libavcodec/Makefile +++ b/libavcodec/Makefile @@ -1019,7 +1019,8 @@ SKIPHEADERS-$(CONFIG_VDA) += vda.h vda_vt_internal.h SKIPHEADERS-$(CONFIG_VDPAU) += vdpau.h vdpau_internal.h SKIPHEADERS-$(CONFIG_VIDEOTOOLBOX) += videotoolbox.h vda_vt_internal.h -TESTPROGS = imgconvert \ +TESTPROGS = avpacket \ + imgconvert \ jpeg2000dwt \ mathops \ options \ diff --git a/libavcodec/tests/avpacket.c b/libavcodec/tests/avpacket.c new file mode 100644 index 0000000..752cf3c --- /dev/null +++ b/libavcodec/tests/avpacket.c @@ -0,0 +1,303 @@ +/* + * This file is part of FFmpeg. + * + * FFmpeg 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. + * + * FFmpeg 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 FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include <stdio.h> +#include <stdlib.h> +#include <inttypes.h> +#include <string.h> +#include "libavcodec/avcodec.h" +#include "libavutil/error.h" + + + + +#define AVPACKET_FIELDS \ + X(static AVBufferRef*, buf, "%p") \ + X(static int64_t, pts, "%" PRId64) \ + X(static int64_t, dts, "%" PRId64) \ + X(static uint8_t*, data, "%p") \ + X(static int, size, "%d") \ + X(static int, stream_index, "%d") \ + X(static int, flags, "%d") \ + X(static AVPacketSideData*, side_data, "%p") \ + X(static int, side_data_elems, "%d") \ + X(static int64_t, duration, "%" PRId64) \ + X(static int64_t, pos, "%" PRId64) + +#define AVBUFFERREF_FIELDS \ + Y(static AVBuffer*, buffer, "%p") \ + Y(static uint8_t*, data, "%p") \ + Y(static int, size, "%d") + +#define AVPACKETSIDEDATA_FIELDS \ + Z(static uint8_t*, data, "%p") \ + Z(static int, size, "%d") \ + Z(static enum AVPacketSideDataType, type, "%d") + + + + +#define AV_MESSAGE_1(format, name, p1, p2) \ + do { \ + const char* str = "cloned variable \"%s\" equals " format \ + " but should be " format "\n"; \ + av_log(NULL, AV_LOG_ERROR, str, #name, p2->name, p1->name ); \ + } while (0) + +#define AV_MESSAGE_2(name, p1, p2) \ + do { \ + const char* str = "contents of cloned variable " \ + "%s equals \"%.*s\"\nbut should be \"%.*s\"\n"; \ + av_log(NULL, AV_LOG_ERROR, str, #name, p1->size, \ + p2->data, p1->size, p1->data); \ + } while(0) + +#define AV_MESSAGE_3(name) \ + do { \ + const char* str = "error! did not attempt to compare " \ + "contents of \"%s\" variable\n"; \ + av_log(NULL, AV_LOG_ERROR, str, #name); \ + } while(0) + +#define AV_MESSAGE_4(string) \ + do { av_log(NULL, AV_LOG_INFO, "%s", string); } while(0) + + + + +#define COMPARE(a, b) ( ((a) == (b)) ? 1 : 0 ) + + + +static void print_AVPacketSideData(AVPacketSideData* sdata) +{ + if(!sdata) + return; +#define Z(type, name, format) \ + av_log(NULL, AV_LOG_INFO, "%*s: " format "\t\n", \ + 25 , #name, sdata->name); \ + \ + if ((strcmp(#name, "data") == 0) \ + && (sdata->size > 0) \ + && sdata->data) \ + av_log(NULL, AV_LOG_INFO, "%*s" "\"%.*s\"\n", 29, "", \ + sdata->size, sdata->data); + + AVPACKETSIDEDATA_FIELDS +#undef Z +} + +static void print_AVBufferRef(AVBufferRef* refdata) +{ + if(!refdata) + return; +#define Y(type, name, format) \ + av_log(NULL, AV_LOG_INFO, "%*s: " format "\t\n", \ + 27, #name, refdata->name); \ + if ((strcmp(#name, "data") == 0) \ + && (refdata->size > 0) \ + && refdata->data) \ + av_log(NULL, AV_LOG_INFO, "%*s" "\"%.*s\"\n", 31, "", \ + refdata->size, refdata->data); + + AVBUFFERREF_FIELDS +#undef Y +} + +static void print_AVPacket(AVPacket* p) +{ + if(!p) + return; + + AV_MESSAGE_4("~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~" + "~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n"); + AV_MESSAGE_4("AVPacket contents:\n\n"); + +#define X(type, name, format) \ + av_log(NULL, AV_LOG_INFO, "%*s: " format "\n", 15, #name, p->name); \ + \ + if(strcmp(#name, "buf") == 0) \ + print_AVBufferRef(p->buf); \ + else if (strcmp(#name, "side_data") == 0) \ + print_AVPacketSideData(p->side_data); \ + if ((strcmp(#name, "data") == 0) \ + && (p->size > 0) \ + && p->data) \ + av_log(NULL, AV_LOG_INFO, "%*s" "\"%.*s\"\n", 19, "", \ + p->size, p->data); + + AVPACKET_FIELDS +#undef X + + AV_MESSAGE_4("~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~" + "~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n"); +} + +static void compare_AVPacketsSideData(AVPacketSideData* sdata1, + AVPacketSideData* sdata2) +{ + if(!sdata1 || !sdata2) + return; + + AV_MESSAGE_4("Comparing AVPacketSideData data memebers...\n"); +#define Z(type, name, format) \ + \ + if (strcmp(#name, "data") == 0) { \ + if((sdata1->size > 0) && (sdata1->size == sdata2->size)) { \ + if(memcmp(sdata1->data, sdata2->data, sdata1->size)) { \ + AV_MESSAGE_2(name, sdata1, sdata2); \ + } \ + } \ + else { \ + AV_MESSAGE_3(name); \ + } \ + } \ + else { \ + if(!COMPARE(sdata1->name, sdata2->name)) { \ + AV_MESSAGE_1(format, name, sdata1, sdata2); \ + } \ + } + AVPACKETSIDEDATA_FIELDS +#undef Z +} + +static void compare_AVPackets(AVPacket* p1, AVPacket* p2) +{ + if(!p1 || !p2) + return; + + AV_MESSAGE_4("Comparing AVPacket data memebers...\n"); +#define X(type, name, format) \ + if(strcmp(#name, "buf") == 0) { \ + \ + } else if (strcmp(#name, "side_data") == 0) { \ + compare_AVPacketsSideData(p1->side_data,p2->side_data); \ + AV_MESSAGE_4("Continue comparing AVPacket data memebers... \n"); \ + } else if (strcmp(#name, "data") == 0) { \ + if((p1->size > 0) && (p1->size == p2->size)) { \ + if(memcmp(p1->data, p2->data, p1->size)) { \ + AV_MESSAGE_2(name, p1, p2); \ + } \ + } \ + else { \ + AV_MESSAGE_3(name); \ + } \ + } \ + else { \ + if(!COMPARE(p1->name, p2->name)) { \ + AV_MESSAGE_1(format, name, p1, p2); \ + } \ + } + AVPACKET_FIELDS + +#undef X +} + +static void setup_side_data_entry(AVPacket* avpkt) +{ + const uint8_t *data_name = NULL; + int ret = 0, bytes; + uint8_t *extra_data = NULL; + + + /* get side_data_name string */ + data_name = av_packet_side_data_name(AV_PKT_DATA_NEW_EXTRADATA); + + /* Allocate a memory bloc */ + bytes = strlen(data_name); + + if(!(extra_data = av_malloc(bytes))){ + ret = AVERROR(ENOMEM); + goto print_error; + } + /* copy side_data_name to extra_data array */ + memcpy(extra_data, data_name, bytes); + + /* create side data for AVPacket */ + ret = av_packet_add_side_data(avpkt, AV_PKT_DATA_NEW_EXTRADATA, + extra_data, bytes); + if(ret < 0){ + goto print_error; + } + + return; + +print_error: + fprintf(stderr, "Error occurred: %s\n", av_err2str(ret)); + exit(1); +} + + +static void initializations(AVPacket* avpkt) +{ + static uint8_t data[] = "selftest for av_packet_clone(...)"; + + /* initialize avpkt */ + av_init_packet(avpkt); + + /* set values for avpkt */ + avpkt->pts = 17; + avpkt->dts = 2; + avpkt->data = data; + avpkt->size = strlen(data); + avpkt->flags = AV_PKT_FLAG_DISCARD; + avpkt->duration = 100; + avpkt->pos = 3; + + setup_side_data_entry(avpkt); +} + + + + +static void test_av_packet_clone(void) +{ + AVPacket avpkt; + AVPacket *avpkt_clone = NULL; + + initializations(&avpkt); + + /* clone avpkt */ + avpkt_clone = av_packet_clone(&avpkt); + + if(avpkt_clone) { + /* compare avpkt and avpkt_clone*/ + compare_AVPackets(&avpkt, avpkt_clone); + AV_MESSAGE_4("Done comparing\n"); + + /* view AVPacket contents */ + AV_MESSAGE_4("Viewing AVPackets...\n"); + print_AVPacket(&avpkt); + print_AVPacket(avpkt_clone); + + /* cleanup */ + av_packet_free(&avpkt_clone); + av_packet_unref(&avpkt); + } + else { + av_log(NULL, AV_LOG_ERROR, "av_packet_clone error\n"); + exit(1); + } +} + +int main(void) +{ + test_av_packet_clone(); + + return 0; +} diff --git a/tests/fate/libavcodec.mak b/tests/fate/libavcodec.mak index cf25285..3bc74c1 100644 --- a/tests/fate/libavcodec.mak +++ b/tests/fate/libavcodec.mak @@ -1,3 +1,8 @@ +FATE_LIBAVCODEC-yes += fate-avpacket +fate-avpacket: libavcodec/tests/avpacket$(EXESUF) +fate-avpacket: CMD = run libavcodec/tests/avpacket +fate-avpacket: REF = /dev/null + FATE_LIBAVCODEC-$(CONFIG_CABAC) += fate-cabac fate-cabac: libavcodec/tests/cabac$(EXESUF) fate-cabac: CMD = run libavcodec/tests/cabac
Function(s) Tested: av_packet_clone(). This test checks if av_packet_clone() can successfully make a copy of an AVPacket. Compares all data members in AVPacket EXCEPT for "buf" because "buf" is initialized to NIL in the original AVPacket [to be cloned]. This test also prints out the all the contents of the original and cloned AVPackets. Signed-off-by: Thomas Turner <thomastdt@googlemail.com> --- libavcodec/Makefile | 3 +- libavcodec/tests/avpacket.c | 303 ++++++++++++++++++++++++++++++++++++++++++++ tests/fate/libavcodec.mak | 5 + 3 files changed, 310 insertions(+), 1 deletion(-) create mode 100644 libavcodec/tests/avpacket.c