[FFmpeg-devel] [PATCH] lavf/segment: add reference_stream option

[Alexander Strasser]

Stefano Sabatini wrote:
> On date Wednesday 2012-12-26 22:40:58 +0100, Alexander Strasser encoded:
> > Stefano Sabatini wrote:
[...]
> > > Will fix it.
> > > 
> > > > Move somewhere else?
> > > 
> > > I'd prefer to keep it in context.
> > 
> >   What about this (sorry neither compiled nor tested):
> > 
> 
> > /* higher value higher priority, 
> >    types not explicitly initialized will never be selected */
> > static const int media_type_priority[AVMEDIA_TYPE_NB] = {
> >   [AVMEDIA_TYPE_VIDEO]      = 5,
> >   [AVMEDIA_TYPE_AUDIO]      = 4,
> >   [AVMEDIA_TYPE_SUBTITLE]   = 3,
> >   [AVMEDIA_TYPE_DATA]       = 2,
> >   [AVMEDIA_TYPE_ATTACHMENT] = 1
> > };
> 
> I prefer to keep a list for semantical consistency (it is also easier
> to update).

  Not sure I really understand; also updating should not be too frequent
action. But you mostly work on this code, so if you like it better it
should be OK.
 
> > int cur_reference_prio = 0;
> > 
> > for (i = 0; i < s->nb_streams; ++i) {
> >    enum AVMediaType media_type = s->streams[i]->codec->codec_type; 
> >    int prio;
> > 
> >    if (media_type < 0 || media_type >= AVMEDIA_TYPE_NB) {
> >      continue;
> >    }
> >    prio = media_type_priority[media_type];
> > 
> >    if (prio > cur_reference_prio) {
> >       seg->reference_stream_index = i;
> >       cur_reference_prio = prio;
> >    }
> > }
> > 
[...]
> -    int ret, i;
> +    int ret, i, j;
>  
>      seg->segment_count = 0;
>      if (!seg->write_header_trailer)
> @@ -398,14 +400,57 @@ static int seg_write_header(AVFormatContext *s)
>      if (seg->list_type == LIST_TYPE_EXT)
>          av_log(s, AV_LOG_WARNING, "'ext' list type option is deprecated in favor of 'csv'\n");
>  
> -    for (i = 0; i < s->nb_streams; i++)
> -        seg->has_video +=
> -            (s->streams[i]->codec->codec_type == AVMEDIA_TYPE_VIDEO);
> +    seg->reference_stream_index = -1;
> +    if (!strcmp(seg->reference_stream_specifier, "auto")) {
> +        /* select first index of type with highest priority */
> +        int type_index_map[AVMEDIA_TYPE_NB];
> +        static const enum AVMediaType type_priority_list[] = {
> +            AVMEDIA_TYPE_VIDEO,
> +            AVMEDIA_TYPE_AUDIO,
> +            AVMEDIA_TYPE_SUBTITLE,
> +            AVMEDIA_TYPE_DATA,
> +            AVMEDIA_TYPE_ATTACHMENT
> +        };
> +        enum AVMediaType type;
> +
> +        for (i = 0; i < AVMEDIA_TYPE_NB; i++)
> +            type_index_map[i] = -1;
> +
> +        /* select first index for each type */
> +        for (i = 0; i < s->nb_streams; i++) {
> +            type = s->streams[i]->codec->codec_type;
> +            if (type_index_map[type] == -1)
> +                type_index_map[type] = i;
> +        }

  Is it safe enough to assume type can never be out of range here?

> +
> +        for (i = 0; i < FF_ARRAY_ELEMS(type_priority_list); j++) {

   How does the variable j fit into the picture?

> +            type = type_priority_list[i];
> +            if ((seg->reference_stream_index = type_index_map[type]) >= 0)
> +                break;
> +        }

   Despite above comments it looks better this time. It still feels
a bit complex to me for a rather non-primary task.

[...]

  Remaining changes look fine to the best of my knowledge. I did not test.
Would probably be good to test on inputs with multiple video streams, with
exactly one video stream and with no videostream at all.


  Alexander