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

[Steve Lhomme]

On 2020-11-16 15:00, Romain Vimont wrote:
> 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.

You're right. It can be simplified. For now I separated the progressive 
and numfield values as one may come from the picture or the video format 
and vice versa depending on how we chose to do it in the end.

BTW we already set the b_progressive to false in that case so 4/9 
wouldn't need changes.