Re: [vlc-devel] [PATCH] Add 0x40 descriptor support

Hi,

This patch belongs on the libdvbpsi-devel mailinglist

Thanks for the patch.

My review comments are in between your patch.

On Tue, Nov 22, 2011 at 2:37 PM, Roberto Corno <[email protected]> wrote:
> ---
>  src/descriptors/dr.h    |    1 +
>  src/descriptors/dr_40.c |   90 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/descriptors/dr_40.h |   86 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 177 insertions(+), 0 deletions(-)
>  create mode 100644 src/descriptors/dr_40.c
>  create mode 100644 src/descriptors/dr_40.h
>
> diff --git a/src/descriptors/dr.h b/src/descriptors/dr.h
> index faf318a..93b27ee 100644
> --- a/src/descriptors/dr.h
> +++ b/src/descriptors/dr.h
> @@ -46,6 +46,7 @@
>  #include "dr_0d.h"
>  #include "dr_0e.h"
>  #include "dr_0f.h"
> +#include "dr_40.h"
>  #include "dr_42.h"
>  #include "dr_43.h"
>  #include "dr_44.h"
> diff --git a/src/descriptors/dr_40.c b/src/descriptors/dr_40.c
> new file mode 100644
> index 0000000..0ae431e
> --- /dev/null
> +++ b/src/descriptors/dr_40.c
> @@ -0,0 +1,90 @@
> +
> +

Missing License, Author and or Copyright notice..
See other descriptors for accepted values.

> +
> +#include "config.h"
> +
> +#include <stdio.h>
> +#include <stdlib.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_40.h"
> +
> +/*****************************************************************************
> + * dvbpsi_DecodeNetworkNameDr
> + *****************************************************************************/
> +dvbpsi_network_name_dr_t* dvbpsi_DecodeNetworkNameDr(
> +                                        dvbpsi_descriptor_t * p_descriptor) {

Coding style violation. You should put the '{' bracket on the next line.

> +       dvbpsi_network_name_dr_t * p_decoded;
> +
> +  /* Check the tag */
> +  if(p_descriptor->i_tag != 0x40)

A space between if and ( is wanted here.

> +  {
> +       DVBPSI_ERROR_ARG("dr_40 decoder", "bad tag (0x%x)", p_descriptor->i_tag);
> +       return NULL;
> +  }
> +  /* Don't decode twice */
> +  if(p_descriptor->p_decoded)
> +       return p_descriptor->p_decoded;
> +  /* Allocate memory */
> +  p_decoded =
> +        (dvbpsi_network_name_dr_t*)malloc(sizeof(dvbpsi_network_name_dr_t));

Use calloc(1, sizeof(dvbpsi_network_name_dr_t)) here

> +  if(!p_decoded)
> +  {
> +    DVBPSI_ERROR("dr_40 decoder", "out of memory");
> +    return NULL;
> +  }
> +  /* Decode data */
> +  p_decoded->i_name_length = p_descriptor->i_length;

check if p_descriptor->i_length does not exceed 255

> +  if(p_decoded->i_name_length)
> +    memcpy(p_decoded->i_name_byte,
> +           p_descriptor->p_data,
> +           p_decoded->i_name_length);

Are you sure p_descriptor->p_data is not NULL ??

> +
> +  p_descriptor->p_decoded = (void*)p_decoded;
> +
> +  return p_decoded;
> +}
> +
> +
> +/*****************************************************************************
> + * dvbpsi_GenNetworkNameDr
> + *****************************************************************************/
> +dvbpsi_descriptor_t * dvbpsi_GenNetworkNameDr(
> +                                        dvbpsi_network_name_dr_t * p_decoded,
> +                                        int b_duplicate) {
> +  /* Create the descriptor */
> +  dvbpsi_descriptor_t * p_descriptor =
> +               dvbpsi_NewDescriptor(0x40, p_decoded->i_name_length, NULL);
> +
> +  if(p_descriptor)
> +  {
> +    /* Encode data */
> +    if(p_decoded->i_name_length)
> +      memcpy(p_descriptor->p_data,
> +             p_decoded->i_name_byte,
> +             p_decoded->i_name_length);

check p_decoded->i_name_length to not be larger then 255.

> +
> +    if(b_duplicate)
> +    {
> +      /* Duplicate decoded data */
> +       dvbpsi_network_name_dr_t * p_dup_decoded =
> +        (dvbpsi_network_name_dr_t*)malloc(sizeof(dvbpsi_network_name_dr_t));
> +      if(p_dup_decoded)

Use calloc(1, sizeof(dvbpsi_network_name_dr_t)); here.

Check for p_dub_decoded is not NULL, if it is clean up and return
NULL, else do the following below.

> +        memcpy(p_dup_decoded, p_decoded, sizeof(dvbpsi_network_name_dr_t));
> +
> +      p_descriptor->p_decoded = (void*)p_dup_decoded;
> +    }
> +  }
> +
> +  return p_descriptor;
> +}
> diff --git a/src/descriptors/dr_40.h b/src/descriptors/dr_40.h
> new file mode 100644
> index 0000000..4c9c8ee
> --- /dev/null
> +++ b/src/descriptors/dr_40.h
> @@ -0,0 +1,86 @@
> +/*
> + * dr_40.h
> + *
> + *  Created on: Nov 22, 2011
> + *      Author: rcorno
> + */

Missing License, Author and or Copyright notice..

> +
> +/*!
> + * \file <dr_40.h>
> + * \author Corno Roberto <[email protected]>
> + * \brief Application interface for the DVB "network name"
> + * descriptor decoder and generator.
> + *
> + * Application interface for the DVB "network name" descriptor
> + * decoder and generator. This descriptor's definition can be found in
> + * ETSI EN 300 468 section 6.2.27.
> + */
> +
> +#ifndef DR_40_H_
> +#define DR_40_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/*****************************************************************************
> + * dvbpsi_network_name_dr_t
> + *****************************************************************************/
> +/*!
> + * \struct dvbpsi_network_name_dr_t
> + * \brief "network name" descriptor structure.
> + *
> + * This structure is used to store a decoded "network name"
> + * descriptor. (ETSI EN 300 468 section 6.2.27).
> + */
> +/*!
> + * \typedef struct dvbpsi_network_nameg_dr_s dvbpsi_network_name_dr_t
> + * \brief dvbpsi_network_name_dr_t type definition.
> + */
> +typedef struct dvbpsi_network_name_dr_s
> +{
> +  uint8_t      i_name_length;            /*!< length of the i_name_byte
> +                                                  array */
> +  uint8_t      i_name_byte[255];         /*!< the name of the delivery system */

Is the name of the delivery system always < 256 characters?

i_name_length could be just i_length (<=== my preference).

i_name_byte[] is the wrong variable name, it should be p_byte or
p_name (although it is technically not a pointer here, but refers
array of bytes).

> +
> +} dvbpsi_network_name_dr_t;
> +
> +/*****************************************************************************
> + * dvbpsi_DecodeNetworkNameDr
> + *****************************************************************************/
> +/*!
> + * \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_network_name_dr_t* dvbpsi_DecodeNetworkNameDr(
> +                                        dvbpsi_descriptor_t * p_descriptor);
> +
> +
> +/*****************************************************************************
> + * dvbpsi_GenNetworkNameDr
> + *****************************************************************************/
> +/*!
> + * \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.
> + */
> +dvbpsi_descriptor_t * dvbpsi_GenNetworkNameDr(
> +                                        dvbpsi_network_name_dr_t * p_decoded,
> +                                        int b_duplicate);

'int b_duplicate' should be a bool now, thus 'bool b_duplicate'.

> +
> +#ifdef __cplusplus
> +};
> +#endif
drop the #ifdef __cplusplus .. #endif defines

It is not done in the entire library, so starting here is of little
use for that.

> +
> +#else
> +#error "Multiple inclusions of dr_40.h"
> +#endif
> --
> 1.7.0.4
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel

Kind regards,

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