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

[Nico Sabbi]

Benoit Fouet wrote:

>>+{
>>+    av_free(ts->prg);
>>+    ts->prg = NULL;
>>  
>>    
>>
> +static void clear_programs(MpegTSContext *ts)
>
>
>av_freep(&ts->prg) does that
>  
>
ok

>>+    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
>  
>

but it would create one more level of bracing and nesting,
making the code less readable

>  
>
>>+            //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]
>  
>
>  
>
>>             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;
>
>  
>

as above: there's one more level of nesting.
Generally I find code with less nesting more readable,
thus discarding all error cases before making something
actually useful is the result

>>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")
>  
>
such as this?

+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;



>  
>
>> #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 ?
>  
>

maybe. I forgot it

>  
>
>>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 ()
>  
>

c&p from mpegts.c, trying to minimize changes to the tiniest essential

>  
>
>>+    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 ;)
>  
>
uhm, yes