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

[Stefano Sabatini]

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 test it this time, updated patch in attachment. I'm going to push
this tomorrow if I read no more comments, thanks for the reviews.
-- 
FFmpeg = Frightening & Fundamental Mere Programmable Exuberant Gem
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-lavf-segment-add-reference_stream-option.patch
Type: text/x-diff
Size: 6029 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121227/a791ec26/attachment.bin>