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

[Stefano Sabatini]

On date Thursday 2012-12-27 22:27:39 +0100, Stefano Sabatini encoded:
> On date Thursday 2012-12-27 01:04:59 +0100, Alexander Strasser encoded:
> > 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.
> 
> I mean that the simplest way to represent a priority list is ... a
> list, thus I prefer to keep it that way.
> 
> >  
> > > > 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?
> 
> Good point.
>  
> > > +
> > > +        for (i = 0; i < FF_ARRAY_ELEMS(type_priority_list); j++) {
> > 
> >    How does the variable j fit into the picture?
> 
> Fixed.
> 
> > 
> > > +            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.
> 
> I tested it this time, updated patch in attachment. I'm going to push
> this tomorrow if I read no more comments, thanks for the reviews.

Applied.
-- 
FFmpeg = Funny Fast Mastodontic Portentous Enlightened Gigant