Re: [FFmpeg-devel] [PATCH] Add new test for libavutil/mastering_display_metadata

[Michael Niedermayer]

On Wed, Mar 23, 2016 at 02:37:31PM +, Petru Rares Sincraian wrote:
> If I understand well you want me to do a test that given a multimedia file 
> test if it loads works ok. If so how can I get a simple multimedia file?
> 

i dont know, but there are probably 3 ways or so
1. search for such a file
2. create such a file
3. ask others where to find one

for all these the first step is probably to identify what exactly
it is that the file needs to have to trigger the code


> I see this tests more like a unitary tests. In my opinion, test if 
> loading/storing to AVFrame need to be done in AVFrame.

i dont mind, it just seems to me the amount of code under test is
disproportionally small compared to the code using to test it



> 
> Thanks :)
> 
> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> on behalf of Michael 
> Niedermayer <mich...@niedermayer.cc>
> Sent: Tuesday, March 22, 2016 4:17 PM
> To: FFmpeg development discussions and patches
> Subject: Re: [FFmpeg-devel] [PATCH] Add new test for
> libavutil/mastering_display_metadata
> 
> On Tue, Mar 22, 2016 at 01:30:11PM +, Petru Rares Sincraian wrote:
> >
> > Hi there,
> >
> > I added a set of tests for libavutil/mastering_display_metadata module. I 
> > attached the patch in this message.
> >
> >
> > Thanks,
> > Petru Rares.
> 
> >  libavutil/Makefile|1
> >  libavutil/mastering_display_metadata.c|   68 
> > ++
> >  libavutil/mastering_display_metadata.h|1
> >  tests/fate/libavutil.mak  |4 +
> >  tests/ref/fate/mastering_display_metadata |1
> >  5 files changed, 74 insertions(+), 1 deletion(-)
> > 3125db1eb98ac3ad3393e88613a90af79ae812b1  
> > 0001-Add-selftest-to-libavutil-mastering_display_metadata.patch
> > From 1e502305f098c9aef852e19e91ddee831cc5ebaf Mon Sep 17 00:00:00 2001
> > From: Petru Rares Sincraian <psincra...@outlook.com>
> > Date: Tue, 22 Mar 2016 11:39:08 +0100
> > Subject: [PATCH] Add selftest to libavutil/mastering_display_metadata
> >
> > This commit adds tests for functions of 
> > libavutil/mastering_display_metadata.c
> > ---
> >  libavutil/Makefile|  1 +
> >  libavutil/mastering_display_metadata.c| 68 
> > +++
> >  libavutil/mastering_display_metadata.h|  1 +
> >  tests/fate/libavutil.mak  |  4 ++
> >  tests/ref/fate/mastering_display_metadata |  0
> >  5 files changed, 74 insertions(+)
> >  create mode 100644 tests/ref/fate/mastering_display_metadata
> >
> > diff --git a/libavutil/Makefile b/libavutil/Makefile
> > index 58df75a..3d89335 100644
> > --- a/libavutil/Makefile
> > +++ b/libavutil/Makefile
> > @@ -198,6 +198,7 @@ TESTPROGS = adler32 
> > \
> >  parseutils  \
> >  pixdesc \
> >  pixelutils  \
> > +mastering_display_metadata  \
> >  random_seed \
> >  rational\
> >  ripemd  \
> > diff --git a/libavutil/mastering_display_metadata.c 
> > b/libavutil/mastering_display_metadata.c
> > index e1683e5..8c264a2 100644
> > --- a/libavutil/mastering_display_metadata.c
> > +++ b/libavutil/mastering_display_metadata.c
> > @@ -41,3 +41,71 @@ AVMasteringDisplayMetadata 
> > *av_mastering_display_metadata_create_side_data(AVFra
> >
> >  return (AVMasteringDisplayMetadata *)side_data->data;
> >  }
> > +
> > +#ifdef TEST
> > +
> > +static int check_alloc(void)
> > +{
> > +int result = 0;
> > +AVMasteringDisplayMetadata *original = 
> > av_mastering_display_metadata_alloc();
> > +
> > +if (original == NULL) {
> > +printf("Failed to allocate display metadata\n");
> > +result = 1;
> > +}
> > +
> > +if (original)
> > +av_freep(original);
> > +
> > +return result;
> > +}
> > +
> > +static int check_create_side_data(void)
> > +{
> > +int result = 0;
> > +AVFrame *frame = av_frame_alloc();
> > +AVMasteringDisplayMet

Re: [FFmpeg-devel] [PATCH] Add new test for libavutil/mastering_display_metadata

[Petru Rares Sincraian]

If I understand well you want me to do a test that given a multimedia file test 
if it loads works ok. If so how can I get a simple multimedia file?

I see this tests more like a unitary tests. In my opinion, test if 
loading/storing to AVFrame need to be done in AVFrame.

Thanks :)

From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> on behalf of Michael 
Niedermayer <mich...@niedermayer.cc>
Sent: Tuesday, March 22, 2016 4:17 PM
To: FFmpeg development discussions and patches
Subject: Re: [FFmpeg-devel] [PATCH] Add new test for    
libavutil/mastering_display_metadata

On Tue, Mar 22, 2016 at 01:30:11PM +, Petru Rares Sincraian wrote:
>
> Hi there,
>
> I added a set of tests for libavutil/mastering_display_metadata module. I 
> attached the patch in this message.
>
>
> Thanks,
> Petru Rares.

>  libavutil/Makefile|1
>  libavutil/mastering_display_metadata.c|   68 
> ++
>  libavutil/mastering_display_metadata.h|1
>  tests/fate/libavutil.mak  |4 +
>  tests/ref/fate/mastering_display_metadata |1
>  5 files changed, 74 insertions(+), 1 deletion(-)
> 3125db1eb98ac3ad3393e88613a90af79ae812b1  
> 0001-Add-selftest-to-libavutil-mastering_display_metadata.patch
> From 1e502305f098c9aef852e19e91ddee831cc5ebaf Mon Sep 17 00:00:00 2001
> From: Petru Rares Sincraian <psincra...@outlook.com>
> Date: Tue, 22 Mar 2016 11:39:08 +0100
> Subject: [PATCH] Add selftest to libavutil/mastering_display_metadata
>
> This commit adds tests for functions of libavutil/mastering_display_metadata.c
> ---
>  libavutil/Makefile|  1 +
>  libavutil/mastering_display_metadata.c| 68 
> +++
>  libavutil/mastering_display_metadata.h|  1 +
>  tests/fate/libavutil.mak  |  4 ++
>  tests/ref/fate/mastering_display_metadata |  0
>  5 files changed, 74 insertions(+)
>  create mode 100644 tests/ref/fate/mastering_display_metadata
>
> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index 58df75a..3d89335 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -198,6 +198,7 @@ TESTPROGS = adler32   
>   \
>  parseutils  \
>  pixdesc \
>  pixelutils  \
> +mastering_display_metadata  \
>  random_seed \
>  rational\
>  ripemd  \
> diff --git a/libavutil/mastering_display_metadata.c 
> b/libavutil/mastering_display_metadata.c
> index e1683e5..8c264a2 100644
> --- a/libavutil/mastering_display_metadata.c
> +++ b/libavutil/mastering_display_metadata.c
> @@ -41,3 +41,71 @@ AVMasteringDisplayMetadata 
> *av_mastering_display_metadata_create_side_data(AVFra
>
>  return (AVMasteringDisplayMetadata *)side_data->data;
>  }
> +
> +#ifdef TEST
> +
> +static int check_alloc(void)
> +{
> +int result = 0;
> +AVMasteringDisplayMetadata *original = 
> av_mastering_display_metadata_alloc();
> +
> +if (original == NULL) {
> +printf("Failed to allocate display metadata\n");
> +result = 1;
> +}
> +
> +if (original)
> +av_freep(original);
> +
> +return result;
> +}
> +
> +static int check_create_side_data(void)
> +{
> +int result = 0;
> +AVFrame *frame = av_frame_alloc();
> +AVMasteringDisplayMetadata *metadata;
> +AVFrameSideData *side_data;
> +
> +if (frame == NULL) {
> +printf("Failed to allocate frame");
> +result = 1;
> +goto end;
> +}
> +
> +metadata = av_mastering_display_metadata_create_side_data(frame);
> +if (metadata == NULL) {
> +printf("Failed to create display metadata frame side data");
> +result = 1;
> +goto end;
> +}
> +
> +side_data = av_frame_get_side_data(frame, 
> AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
> +if (side_data == NULL) {
> +printf("Failed to get frame side data");
> +result = 1;
> +goto end;
> +}

i think to test side data handling the code should extract and display
it based on a sample multimedia file, like using ffprobe.
thats also more generic and can work with any side data case
that would also test if storing an

Re: [FFmpeg-devel] [PATCH] Add new test for libavutil/mastering_display_metadata

[Michael Niedermayer]

On Tue, Mar 22, 2016 at 01:30:11PM +, Petru Rares Sincraian wrote:
> 
> Hi there, 
> 
> I added a set of tests for libavutil/mastering_display_metadata module. I 
> attached the patch in this message.
> 
> 
> Thanks,
> Petru Rares.

>  libavutil/Makefile|1 
>  libavutil/mastering_display_metadata.c|   68 
> ++
>  libavutil/mastering_display_metadata.h|1 
>  tests/fate/libavutil.mak  |4 +
>  tests/ref/fate/mastering_display_metadata |1 
>  5 files changed, 74 insertions(+), 1 deletion(-)
> 3125db1eb98ac3ad3393e88613a90af79ae812b1  
> 0001-Add-selftest-to-libavutil-mastering_display_metadata.patch
> From 1e502305f098c9aef852e19e91ddee831cc5ebaf Mon Sep 17 00:00:00 2001
> From: Petru Rares Sincraian 
> Date: Tue, 22 Mar 2016 11:39:08 +0100
> Subject: [PATCH] Add selftest to libavutil/mastering_display_metadata
> 
> This commit adds tests for functions of libavutil/mastering_display_metadata.c
> ---
>  libavutil/Makefile|  1 +
>  libavutil/mastering_display_metadata.c| 68 
> +++
>  libavutil/mastering_display_metadata.h|  1 +
>  tests/fate/libavutil.mak  |  4 ++
>  tests/ref/fate/mastering_display_metadata |  0
>  5 files changed, 74 insertions(+)
>  create mode 100644 tests/ref/fate/mastering_display_metadata
> 
> diff --git a/libavutil/Makefile b/libavutil/Makefile
> index 58df75a..3d89335 100644
> --- a/libavutil/Makefile
> +++ b/libavutil/Makefile
> @@ -198,6 +198,7 @@ TESTPROGS = adler32   
>   \
>  parseutils  \
>  pixdesc \
>  pixelutils  \
> +mastering_display_metadata  \
>  random_seed \
>  rational\
>  ripemd  \
> diff --git a/libavutil/mastering_display_metadata.c 
> b/libavutil/mastering_display_metadata.c
> index e1683e5..8c264a2 100644
> --- a/libavutil/mastering_display_metadata.c
> +++ b/libavutil/mastering_display_metadata.c
> @@ -41,3 +41,71 @@ AVMasteringDisplayMetadata 
> *av_mastering_display_metadata_create_side_data(AVFra
>  
>  return (AVMasteringDisplayMetadata *)side_data->data;
>  }
> +
> +#ifdef TEST
> +
> +static int check_alloc(void)
> +{
> +int result = 0;
> +AVMasteringDisplayMetadata *original = 
> av_mastering_display_metadata_alloc();
> +
> +if (original == NULL) {
> +printf("Failed to allocate display metadata\n");
> +result = 1;
> +}
> +
> +if (original)
> +av_freep(original);
> +
> +return result;
> +}
> +
> +static int check_create_side_data(void)
> +{
> +int result = 0;
> +AVFrame *frame = av_frame_alloc();
> +AVMasteringDisplayMetadata *metadata;
> +AVFrameSideData *side_data;
> +
> +if (frame == NULL) {
> +printf("Failed to allocate frame");
> +result = 1;
> +goto end;
> +}
> +
> +metadata = av_mastering_display_metadata_create_side_data(frame);
> +if (metadata == NULL) {
> +printf("Failed to create display metadata frame side data");
> +result = 1;
> +goto end;
> +}
> +
> +side_data = av_frame_get_side_data(frame, 
> AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
> +if (side_data == NULL) {
> +printf("Failed to get frame side data");
> +result = 1;
> +goto end;
> +}

i think to test side data handling the code should extract and display
it based on a sample multimedia file, like using ffprobe.
thats also more generic and can work with any side data case
that would also test if storing and loading sidedata to a AVFrame
works

This test here seems very specific, testing adding one specific type
of sidedata to a struct and retrieving it again.
If sidedata store/load would be tested, all should likely be tested
but then that should already be tested by testing some media file

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel