[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:07, Steve Lhomme wrote:
> 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.

There is a small issue though. For now this patchset only takes in 
account avcodec. There are other (hardware) decoders capable of decoding 
HEVC. For testing it seems nvdec also doesn't merge fields by itself (or 
detect interlacing or supports interlacing). It will also need to set 
the proper number of fields (patch somewhere in a branch). Except nvdec 
is supposed to output progressive pictures by default (there's a mode 
that's supposed not to alter interlaced content too) and always sets the 
b_progressive flag to true. This will need to be checked too.

All decoders will need to be checked what they output (most likely a 
single field) and set the proper number of fields for HEVC and the 
interlaced flag even though they never did so far. If the flag is set by 
the packetizer in the video format, the decoder may just forward that 
information in the picture it outputs. So it could theoretically be 
possible to output 1 field and not have the b_progressive flag set to 
false (because it's only found in the picture). Such decoders will now 
need to be double checked to modify the picture fields they output.