[FFmpeg-devel] [PATCH] demux individual program out of MPEG-TS

[Benoit Fouet]

Nico Sabbi wrote:
> Index: libavformat/mpegts.c
> ===================================================================
> --- libavformat/mpegts.c	(revisione 10435)
> +++ libavformat/mpegts.c	(copia locale)
> @@ -109,8 +109,9 @@
>      /** number of PMTs in the last PAT seen                  */
>      int nb_services;
>      /** list of PMTs in the last PAT seen                    */
> -    MpegTSService **services;
>  
> +    unsigned int nb_prg;
> +    Program_t *prg;
>   

doesn't the doxygen comment have to be changed too ?

> +static void clear_programs(MpegTSContext *ts)
> +{
> +    av_free(ts->prg);
> +    ts->prg = NULL;
>   

av_freep(&ts->prg) does that

> +static int discard_pid(MpegTSContext *ts, unsigned int pid)
> +{
> +    //returns 1 if the pid is only comprised in programs that have .discard=AVDISCAD_ALL
> +    //0 othewise
>   

nits: AVDISCARD_ALL, otherwise
and maybe this should be placed before the function, and doxygen compliant

> +    int i, j, k;
> +    int used = 0, discarded = 0;
> +    Program_t *p;
> +    for(i=0; i<ts->nb_prg; i++) {
> +        p = &ts->prg[i];
> +        for(j=0; j<p->nb_pids; j++) {
> +            if(p->pids[j] != pid)
> +                continue;
>   

using if(p->pids[j] == pid) would get rid of the continue

> +            //is program with id p->id set to be discarded?
> +            for(k=0; k<ts->stream->nb_programs; k++) {
> +                if(ts->stream->programs[k]->id == p->id) {
> +                    if(ts->stream->programs[k]->discard == AVDISCARD_ALL)
> +                        discarded++;
> +                    else
> +                        used++;
> +                }
> +            }
> +        }
> +    }
> +
> +    return (!used && discarded);
> +}
> +
>   

[snip]

> @@ -666,6 +715,7 @@
>                     desc_tag, desc_len);
>  #endif
>              switch(desc_tag) {
> +            AVProgram *program = NULL;
>   

can be removed (see below)

>              case 0x48:
>                  service_type = get8(&p, p_end);
>                  if (service_type < 0)
> @@ -676,7 +726,9 @@
>                  name = getstr8(&p, p_end);
>                  if (!name)
>                      break;
> -                new_service(ts, sid, provider_name, name);
> +                program = av_new_program(ts->stream, sid);
> +                if(program)
> +                    av_set_program_name(program, provider_name, name);
>                  break;
>   

how about:
name = getstr8(&p, p_end);
if (name) {
    AVProgram *program = av_new_program(ts->stream, sid);
    if (program)
        av_set_program_name(program, provider_name, name);
}
break;

> Index: libavformat/avformat.h
> ===================================================================
> --- libavformat/avformat.h	(revisione 10435)
> +++ libavformat/avformat.h	(copia locale)
> @@ -345,6 +345,14 @@
>      int64_t pts_buffer[MAX_REORDER_DELAY+1];
>  } AVStream;
>  
> +typedef struct AVProgram {
> +    int            id;
> +    char*          provider_name; //Network name for DVB streams
> +    char*          name;          //Service name for DVB streams
> +    int            running;
> +    enum AVDiscard discard;       //selects which program to discard and which to feed to the caller
> +} AVProgram;
> +
>   

comments are not doxygen compatible
(and i personnaly slightly prefer "char *name" to "char* name")

>  #define AVFMTCTX_NOHEADER      0x0001 /**< signal that no header is present
>                                           (streams are added dynamically) */
>  
> @@ -430,6 +438,9 @@
>  
>      const uint8_t *key;
>      int keylen;
> +
> +    unsigned int nb_programs;
> +    AVProgram **programs;
>  } AVFormatContext;
>  
>  typedef struct AVPacketList {
> @@ -647,6 +658,8 @@
>   * @param id file format dependent stream id
>   */
>  AVStream *av_new_stream(AVFormatContext *s, int id);
> +AVProgram *av_new_program(AVFormatContext *s, int i);
> +void av_set_program_name(AVProgram *program, char *provider_name, char *name);
>  
>  /**
>   * Set the pts for a given stream.
>   

wouldn't all that also need a minor version bump ?

> Index: libavformat/utils.c
> ===================================================================
> --- libavformat/utils.c	(revisione 10435)
> +++ libavformat/utils.c	(copia locale)
> +void av_set_program_name(AVProgram *program, char *provider_name, char *name)
> +{
> +    assert((!provider_name) == (!name));
>   

superflous ()

> +    if(name){
> +        av_free(program->provider_name);
> +        av_free(program->         name);
> +        program->provider_name = provider_name;
> +        program->         name =          name;
>   

if they are to be freed by the function, i'd use av_strdup, or tell
explicitely in the function documentation that the strings passed to
this function are not the caller's property anymore ;)

> Index: ffmpeg.c
> ===================================================================
> --- ffmpeg.c	(revisione 10435)
> +++ ffmpeg.c	(copia locale)
> @@ -184,6 +184,7 @@
>  static int video_global_header = 0;
>  static char *vstats_filename;
>  static FILE *vstats_file;
> +static int opt_programid = 0;
>   

the = 0 is unneeded.

-- 
Ben
Purple Labs S.A.
www.purplelabs.com