[FFmpeg-devel] [PATCH] libavcodec/tests: Added selftest for libavcodec/avpacket.c

Michael Niedermayer michael at niedermayer.cc
Tue Oct 18 18:05:21 EEST 2016


On Mon, Oct 17, 2016 at 11:22:21PM -0700, tdturner at ucdavis.edu wrote:
> From: Thomas Turner <thomastdt at gmail.com>
> 
> Improved code coverage for libavcodec.
> Function(s) Tested: av_packet_clone()
> 
> Signed-off-by: Thomas Turner <thomastdt at gmail.com>
> ---
>  libavcodec/Makefile         |   3 +-
>  libavcodec/tests/avpacket.c | 207 ++++++++++++++++++++++++++++++++++++++++++++
>  tests/fate/libavcodec.mak   |   5 ++
>  3 files changed, 214 insertions(+), 1 deletion(-)
>  create mode 100644 libavcodec/tests/avpacket.c
> 
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index a1560ba..d64b8df 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -1016,7 +1016,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..5b884d7
> --- /dev/null
> +++ b/libavcodec/tests/avpacket.c
> @@ -0,0 +1,207 @@
> +/*
> + * 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"
> +
> +
> +
> +
> +static char* getbuffer(AVPacket avpkt, int index)
> +{
> +    uint8_t *buffer;
> +    int val, tot_elem, buffer_size = 256;
> +
> +    /* Allocate 256 bytes */
> +    if((buffer = malloc(buffer_size)) == NULL){
> +        perror("malloc");
> +        goto EXIT;
> +    }
> +

> +    if(index == 0){
> +        val = snprintf(buffer, buffer_size,
> +            "{buffer: %p,  data: %p, size: %d}",
> +            avpkt.buf->buffer, avpkt.buf->data, avpkt.buf->size);
> +
> +        tot_elem = strlen(buffer);
> +    }
> +    else if(index == 1){
> +        val = snprintf(buffer, buffer_size, "\"%s\"", avpkt.data);
> +        tot_elem = strlen(buffer);
> +    }
> +    else if(index == 2){
> +        val = snprintf(buffer, buffer_size,
> +            "{data: %p \"%s\", size: %d, type: %d}",
> +            avpkt.side_data, avpkt.side_data->data, avpkt.side_data->size,
> +            avpkt.side_data->type);
> +
> +        tot_elem = strlen(buffer);
> +    }

" tot_elem = strlen(buffer);" can be factored out


> +
> +    /* snprintf fail check */
> +    if(!(val > -1 && val < buffer_size)){
> +        perror("snprintf");
> +        free(buffer);
> +        goto EXIT;
> +    }
> +
> +    return buffer;
> +
> +EXIT:
> +    exit(-1);
> +}
> +

> +static void LOG_AVPACKET(AVPacket avpkt, const char* message)

function names in ffmpeg are lower case


> +{
> +    uint8_t *buf_info = 0, *data_info = 0, *side_info = 0;
> +
> +    /* get buf information */
> +    if(avpkt.buf){
> +        buf_info = getbuffer(avpkt, 0);
> +    }
> +
> +    /* get data information */
> +    if(avpkt.data){
> +        data_info = getbuffer(avpkt, 1);
> +    }
> +
> +    /* get side data information */
> +    if(avpkt.side_data){
> +        side_info = getbuffer(avpkt, 2);
> +    }




> +
> +    /* log standard packet information */
> +    av_log(NULL, AV_LOG_INFO,
> +           "\n%s:\n\n"
> +           "buf\t\t: %p "
> +           "%s\n"
> +           "pts\t\t: %" PRId64 "\n"
> +           "dts\t\t: %" PRId64 "\n"
> +           "data\t\t: %p "
> +           "%s\n"
> +           "size\t\t: %d\n"
> +           "stream_index\t: %d\n"
> +           "flags\t\t: %d\n"
> +           "side_data\t: %p "
> +           "%s\n"
> +           "side_data_elems\t: %d\n"
> +           "duration\t: %" PRId64 "\n"
> +           "pos\t\t: %" PRId64 "\n\n",
> +           message,
> +           avpkt.buf,
> +           buf_info,
> +           avpkt.pts,
> +           avpkt.dts,
> +           avpkt.data,
> +           data_info,
> +           avpkt.size,
> +           avpkt.stream_index,
> +           avpkt.flags,
> +           avpkt.side_data,
> +           side_info,
> +           avpkt.side_data_elems,
> +           avpkt.duration,
> +           avpkt.pos
> +    );

printing NULL with %s is undefined behavior, this needs some
check for that case or a default string to replace NULL


> +

> +    /* cleanup */
> +    if(buf_info)
> +        free(buf_info);
> +
> +    if(data_info)
> +        free(data_info);
> +
> +    if(side_info)
> +        free(side_info);

The null checks are not needed


> +}
> +
> +
> +static void TEST_AV_PACKET_CLONE(void)
> +{
> +    int ret = 0, len;
> +    uint8_t *extra_data = NULL;
> +    const uint8_t *data_name = NULL;
> +    AVPacket avpkt, *avpkt_clone = NULL;
> +    uint8_t data[] = "selftest for av_packet_clone(...)";
> +
> +    /* initialize avpkt */
> +    av_init_packet(&avpkt);
> +
> +    avpkt.data = data;
> +    avpkt.size = strlen(data);
> +    avpkt.flags = AV_PKT_FLAG_DISCARD;
> +
> +    /* get side_data_name string */
> +    data_name = av_packet_side_data_name(AV_PKT_DATA_NEW_EXTRADATA);
> +
> +    /* Allocate a memory bloc */
> +    len = strlen(data_name);
> +    extra_data = av_malloc(len);
> +
> +    if(!extra_data){
> +        ret = AVERROR(ENOMEM);
> +        goto print_error;
> +    }
> +    /* copy side_data_name to extra_data array */
> +    memcpy(extra_data, data_name, len);
> +
> +    /* create side data for AVPacket */
> +    ret = av_packet_add_side_data(&avpkt, AV_PKT_DATA_NEW_EXTRADATA,
> +                                        extra_data, len);
> +    if(ret < 0){
> +        goto print_error;
> +    }
> +
> +    /* clone avpkt */
> +    avpkt_clone = av_packet_clone(&avpkt);
> +
> +    /* log packet information */
> +    if(avpkt_clone){
> +        LOG_AVPACKET(avpkt, "Original Packet info");
> +        LOG_AVPACKET(*avpkt_clone, "Cloned Packet info");
> +    }
> +    else{
> +        av_log(NULL, AV_LOG_ERROR, "av_packet_clone error\n");
> +        goto fail;
> +    }

The code should check that the packet content has not changed
ATM this test prints this information but doesnt compare it against
anything, also the pointer values cannot be compared as they would be
different in each run

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161018/5bf2f413/attachment.sig>


More information about the ffmpeg-devel mailing list