[vlc-devel] [PATCH 1/9] video_format: add the number of fields per picture in the video format

[Romain Vimont]

On Mon, Nov 16, 2020 at 02:11:35PM +0100, Steve Lhomme wrote:
> On 2020-11-16 14:00, Romain Vimont wrote:
> > On Mon, Nov 16, 2020 at 01:44:12PM +0100, Steve Lhomme wrote:
> > > In general it's 2 fields. If the number of fields changes in the source, the
> > > (deinterlacing) filters should be updated.
> > > 
> > > By default the video format is progressive and the i_num_fields can be ignored.
> > > It should be set to 2 for progressive to be consistent with how the value was
> > > set so far.
> > > 
> > > This field is more stable than the picture i_nb_fields that also included
> > > repeated fields and pictures. It only contains the information about the
> > > number of fields in each actual picture.
> > > ---
> > >   include/vlc_es.h                      | 3 +++
> > >   modules/visualization/visual/visual.c | 1 +
> > >   2 files changed, 4 insertions(+)
> > > 
> > > diff --git a/include/vlc_es.h b/include/vlc_es.h
> > > index 404a937d833..459d16b3c37 100644
> > > --- a/include/vlc_es.h
> > > +++ b/include/vlc_es.h
> > > @@ -376,6 +376,8 @@ struct video_format_t
> > >       video_multiview_mode_t multiview_mode;        /** Multiview mode, 2D, 3D */
> > >       bool b_multiview_right_eye_first;   /** Multiview left or right eye first*/
> > > +    unsigned int    i_num_fields;          /**< number of fields per picture */
> > 
> > I think there are only 2 cases:
> >   - either all fields are interleaved in a single picture
> >   - either the fields are split into separate pictures
> 
> Well, a picture could actually have more than 2 fields. It can be 2 and
> repeat the first field. I have yet to understand how this works in the real
> world.
> 
> > 
> > So IMO using an integer adds complexity (more cases to handle) for no
> > reason. I suggest something like a 'bool interlaced_split_fields'
> > instead.
> > 
> > Also, the interlaced/progressive flag should also be in the
> > video_format_t (as you suggested in another thread): having 1 field when
> > the video is progressive is not the same as having 1 field when the
> > video is interlaced.
> 
> That's why I said progressive sources should use a value of 2 (which they
> already do in the picture_t field). That makes things simpler. A value of 1
> means it's interlaced and with one field at a time. That's what allows patch
> 5/9 to trigger the deinterlacer in that case.

I understand your point, but it looks confusing (to me). From
i_num_fields only (especially if it equals 2), we can't know if the
video is interlaced or not, so it seems odd to store i_num_fields but
not a flag progressive/interlaced.


Btw, in patch 5/9, you changed the condition from:

    (C1) !decoded->b_progressive

to

    (C2) !decoded->b_progressive || decoded->i_nb_fields == 1

According to what you just said:

> A value of 1 means it's interlaced and with one field at a time.

    (C3) decoder->i_nb_fields == 1   ==>   !decoded->b_progressive
                                  (implies)

(C3) and (C2) implies (C1).
As a consequence, (C2) can be simplified to (C1), and the patch should
not be necessary.

In other words, if your patch 5/9 is necessary, it means that there are
cases where decoder->i_nb_fields == 1 and decoder->b_progressive (which
contradicts your description of what a value of 1 means).

If this really happens, I think that b_progressive should be set to
false in patch 4 (where you set i_nb_fields=1), and patch 5 discarded.

Regards