Re: [PATCH] Add 0x41 descriptor support

Roberto,

Could you attach this patch as file or create a patch which I can add
with git-am directly?  It would easy applying your patches.

A few remarks below, please correct them.

On Mon, May 21, 2012 at 3:00 PM, Roberto Corno <[email protected]> wrote:
> ---
>  src/Makefile.am         |    9 ++-
>  src/descriptors/dr.h    |    1 +
>  src/descriptors/dr_41.c |  120 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/descriptors/dr_41.h |  104 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 231 insertions(+), 3 deletions(-)
>  create mode 100644 src/descriptors/dr_41.c
>  create mode 100644 src/descriptors/dr_41.h
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 356bbc2..ec07efa 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -4,7 +4,7 @@ pkgincludedir = $(includedir)/dvbpsi
>
>  lib_LTLIBRARIES = libdvbpsi.la
>
> -libdvbpsi_la_SOURCES = dvbpsi.c dvbpsi_private.h \
> +libdvbpsi_la_SOURCES = dvbpsi.c dvbpsi_private.h custom.c \
>                        psi.c \
>                        demux.c \
>                        descriptor.c \
> @@ -13,7 +13,7 @@ libdvbpsi_la_SOURCES = dvbpsi.c dvbpsi_private.h \
>
>  libdvbpsi_la_LDFLAGS = -version-info 8:0:0 -no-undefined
>
> -pkginclude_HEADERS = dvbpsi.h psi.h descriptor.h demux.h \
> +pkginclude_HEADERS = dvbpsi.h psi.h descriptor.h demux.h custom.h \
>                      tables/pat.h tables/pmt.h tables/sdt.h tables/eit.h \
>                      tables/cat.h tables/nit.h tables/tot.h tables/sis.h \
>                     tables/bat.h \
> @@ -31,6 +31,7 @@ pkginclude_HEADERS = dvbpsi.h psi.h descriptor.h demux.h \
>                      descriptors/dr_0d.h \
>                      descriptors/dr_0e.h \
>                      descriptors/dr_0f.h \
> +                     descriptors/dr_41.h \
>                      descriptors/dr_42.h \
>                      descriptors/dr_43.h \
>                      descriptors/dr_44.h \
> @@ -46,7 +47,7 @@ pkginclude_HEADERS = dvbpsi.h psi.h descriptor.h demux.h \
>                      descriptors/dr_59.h \
>                      descriptors/dr_5a.h \
>                      descriptors/dr_69.h \
> -                    descriptors/dr_8a.h \
> +                            descriptors/dr_8a.h \
>                      descriptors/dr.h
>
>  descriptors_src = descriptors/dr_02.c \
> @@ -63,6 +64,8 @@ descriptors_src = descriptors/dr_02.c \
>                   descriptors/dr_0d.c \
>                   descriptors/dr_0e.c \
>                   descriptors/dr_0f.c \
> +                  descriptors/dr_40.c \
> +                  descriptors/dr_41.c \
>                   descriptors/dr_42.c \
>                   descriptors/dr_43.c \
>                   descriptors/dr_44.c \
> diff --git a/src/descriptors/dr.h b/src/descriptors/dr.h
> index 93b27ee..5e71541 100644
> --- a/src/descriptors/dr.h
> +++ b/src/descriptors/dr.h
> @@ -47,6 +47,7 @@
>  #include "dr_0e.h"
>  #include "dr_0f.h"
>  #include "dr_40.h"
> +#include "dr_41.h"
>  #include "dr_42.h"
>  #include "dr_43.h"
>  #include "dr_44.h"
> diff --git a/src/descriptors/dr_41.c b/src/descriptors/dr_41.c
> new file mode 100644
> index 0000000..19f8977
> --- /dev/null
> +++ b/src/descriptors/dr_41.c
> @@ -0,0 +1,120 @@
> +/*
> + * dr_41.c
> + * Copyright (C) 2001-2011 VideoLAN

Copyright (C) 2012 VideoLAN

I doubt you started writting this code in 2001.

> + *
> + * Authors: rcorno (Nov 22, 2011)
> + *
> + * This library 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.
> + *
> + * This library 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 this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + *****************************************************************************/
> +
> +#include "config.h"
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdbool.h>
> +#include <string.h>
> +
> +#if defined(HAVE_INTTYPES_H)
> +#include <inttypes.h>
> +#elif defined(HAVE_STDINT_H)
> +#include <stdint.h>
> +#endif
> +
> +#include "../dvbpsi.h"
> +#include "../dvbpsi_private.h"
> +#include "../descriptor.h"
> +
> +#include "dr_41.h"
> +
> +/*****************************************************************************
> + * dvbpsi_DecodeServiceListDr
> + *****************************************************************************/
> +dvbpsi_service_list_dr_t* dvbpsi_DecodeServiceListDr(
> +                                        dvbpsi_descriptor_t * p_descriptor)
> +{
> +       dvbpsi_service_list_dr_t * p_decoded;
> +       int i;
> +

indentation is not consistent here

> +    /* Check the tag */
> +    if (p_descriptor->i_tag != 0x41)
> +        return NULL;
> +
> +    /* Don't decode twice */
> +    if (p_descriptor->p_decoded)
> +        return p_descriptor->p_decoded;
> +
> +    /* Allocate memory */
> +    p_decoded = (dvbpsi_service_list_dr_t*)calloc(1, sizeof(dvbpsi_service_list_dr_t));
> +    if (!p_decoded)
> +        return NULL;
> +
> +    /* Decode data and check the length */
> +    if((p_descriptor->i_length < 1) || (p_descriptor->i_length % 3 != 0))
> +    {
> +      free(p_decoded);
> +      return NULL;
> +    }

Check length before allocating memory.

> +    p_decoded->i_service_count = p_descriptor->i_length / 3;

Check here if service count is below the maximum of 64.

> +    i=0;
> +    while( i < p_decoded->i_service_count ) {
> +       p_decoded->i_service[i].i_service_id = ((uint16_t)(p_descriptor->p_data[i*3]) << 8)
> +                                      | p_descriptor->p_data[i*3+1];
> +       p_decoded->i_service[i].i_service_type = p_descriptor->p_data[i*3+2];
> +       i++;
> +    }
> +
> +    p_descriptor->p_decoded = (void*)p_decoded;
> +
> +    return p_decoded;
> +}
> +
> +
> +/*****************************************************************************
> + * dvbpsi_GenServiceListDr
> + *****************************************************************************/
> +dvbpsi_descriptor_t * dvbpsi_GenServiceListDr(
> +                                               dvbpsi_service_list_dr_t * p_decoded,
> +                                        bool b_duplicate)

Watch your indentation here.

> +{
> +    /* Create the descriptor */
> +    dvbpsi_descriptor_t * p_descriptor =
> +            dvbpsi_NewDescriptor(0x83, p_decoded->i_service_count*3, NULL);
> +
> +    if (!p_descriptor)
> +        return NULL;
> +
> +    /* Encode data */
> +    int i = 0;

Check if service count is below the maximum of 64, see your header file.

> +    while( i < p_decoded->i_service_count ) {
> +      p_descriptor->p_data[i*3] = p_decoded->i_service[i].i_service_id >> 8;
> +      p_descriptor->p_data[i*3+1] = p_decoded->i_service[i].i_service_id;
> +      p_descriptor->p_data[i*3+2] = p_decoded->i_service[i].i_service_type;
> +      i++;
> +    }
> +
> +    if (b_duplicate)
> +    {
> +        /* Duplicate decoded data */
> +       dvbpsi_service_list_dr_t * p_dup_decoded =
> +                (dvbpsi_service_list_dr_t*)calloc(1, sizeof(dvbpsi_service_list_dr_t));
> +        if(p_dup_decoded)
> +            memcpy(p_dup_decoded, p_decoded, sizeof(dvbpsi_service_list_dr_t));
> +
> +        p_descriptor->p_decoded = (void*)p_dup_decoded;

There is a helper function for this now, please use it:

        p_descriptor->p_decoded =
                dvbpsi_DuplicateDecodedDescriptor(p_descriptor->p_decoded,

   sizeof(dvbpsi_service_list_dr_t));

> +    }
> +
> +    return p_descriptor;
> +}
> diff --git a/src/descriptors/dr_41.h b/src/descriptors/dr_41.h
> new file mode 100644
> index 0000000..749d316
> --- /dev/null
> +++ b/src/descriptors/dr_41.h
> @@ -0,0 +1,104 @@
> +/*****************************************************************************
> + * dr_41.h
> + * Copyright (C) 2001-2011 VideoLAN

Copyright (C) 2012 VideoLAN

> + *
> + * Authors: rcorno (Nov 22, 2011)
> + *
> + * This library 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.
> + *
> + * This library 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 this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + *****************************************************************************/
> +
> +/*!
> + * \file <dr_41.h>
> + * \author Corno Roberto <[email protected]>
> + * \brief Application interface for the DVB "service list"
> + * descriptor decoder and generator.
> + *
> + * Application interface for the DVB "service list" descriptor
> + * decoder and generator. This descriptor's definition can be found in
> + * ETSI EN 300 468 section 6.2.35.
> + */
> +
> +#ifndef DR_41_H_
> +#define DR_41_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/*****************************************************************************
> + * dvbpsi_network_name_dr_t
> + *****************************************************************************/
> +/*!
> + * \struct dvbpsi_service_list_dr_t
> + * \brief "service list" descriptor structure.
> + *
> + * This structure is used to store a decoded "service list"
> + * descriptor. (ETSI EN 300 468 section 6.2.35).
> + */
> +/*!
> + * \typedef struct dvbpsi_service_list_dr_s dvbpsi_service_list_dr_t
> + * \brief dvbpsi_service_list_dr_t type definition.
> + */
> +typedef struct dvbpsi_service_list_dr_s
> +{
> +  uint8_t       i_service_count;            /*!< length of the i_service_list
> +                                                    array */
> +  struct {
> +      uint16_t     i_service_id;            /*!< service id */
> +      uint8_t      i_service_type;          /*!< service type */
> +  } i_service[64];
> +
> +} dvbpsi_service_list_dr_t;
> +
> +/*****************************************************************************
> + * dvbpsi_DecodeServiceListDr
> + *****************************************************************************/
> +/*!
> + * \fn dvbpsi_network_name_dr_t * dvbpsi_DecodeNetworkNameDr(
> +                                        dvbpsi_descriptor_t * p_descriptor)
> + * \brief "network name" descriptor decoder.
> + * \param p_descriptor pointer to the descriptor structure
> + * \return a pointer to a new "network name" descriptor structure
> + * which contains the decoded data.
> + */
> +dvbpsi_service_list_dr_t* dvbpsi_DecodeServiceListDr(
> +                                        dvbpsi_descriptor_t * p_descriptor);
> +

The doxygen comments mentions another function then declared.

> +
> +/*****************************************************************************
> + * dvbpsi_GenServiceListDr
> + *****************************************************************************/
> +/*!
> + * \fn dvbpsi_descriptor_t * dvbpsi_GenNetworkNameDr(
> +                        dvbpsi_network_name_dr_t * p_decoded, int b_duplicate)
> + * \brief "network name" descriptor generator.
> + * \param p_decoded pointer to a decoded "network name" descriptor
> + * structure
> + * \param b_duplicate if non zero then duplicate the p_decoded structure into
> + * the descriptor
> + * \return a pointer to a new descriptor structure which contains encoded data.
> + */

The doxygen comments mentions another function then declared.

> +dvbpsi_descriptor_t * dvbpsi_GenServiceListDr(
> +                                               dvbpsi_service_list_dr_t * p_decoded,
> +                                        bool b_duplicate);

Identation.

> +
> +#ifdef __cplusplus
> +};
> +#endif
> +
> +#else
> +#error "Multiple inclusions of dr_41.h"
> +#endif /* DR_83_H_ */
> --
> 1.7.5.4
>
> _______________________________________________
> libdvbpsi-devel mailing list
> [email protected]
> http://mailman.videolan.org/listinfo/libdvbpsi-devel

Kind regards,

Jean-Paul Saman
_______________________________________________
libdvbpsi-devel mailing list
[email protected]
http://mailman.videolan.org/listinfo/libdvbpsi-devel