[FFmpeg-devel] avisynth.c corrected interlace detection

Sorry for the delay on this, should have corrected it much earlier.
There was some confusion in the interlaced analysis. From 3rdparty decoders perspective, a clip
can only be interlaced when it is NOT field_based. This is because in a field_based clip, the fields
are separate images, so it is actually progressive. In that case, avisynth still shows is_tff or bff because those
values are needed in case the script decides to weave the separated fields later on.

---
 libavformat/avisynth.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

On 5/24/2021 11:03 AM, emcodem wrote:
> Sorry for the delay on this, should have corrected it much earlier.
> There was some confusion in the interlaced analysis. From 3rdparty decoders perspective, a clip
> can only be interlaced when it is NOT field_based. This is because in a field_based clip, the fields
> are separate images, so it is actually progressive. In that case, avisynth still shows is_tff or bff because those
> values are needed in case the script decides to weave the separated fields later on.
> 
> ---
>   libavformat/avisynth.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/libavformat/avisynth.c b/libavformat/avisynth.c
> index 350ac6d11d..ff6e6e1bfa 100644
> --- a/libavformat/avisynth.c
> +++ b/libavformat/avisynth.c
> @@ -250,15 +250,12 @@ static int avisynth_create_stream_video(AVFormatContext *s, AVStream *st)
>       st->nb_frames         = avs->vi->num_frames;
>       avpriv_set_pts_info(st, 32, avs->vi->fps_denominator, avs->vi->fps_numerator);
>   
> -    av_log(s, AV_LOG_TRACE, "avs_is_field_based: %d\n", avs_is_field_based(avs->vi));
> -    av_log(s, AV_LOG_TRACE, "avs_is_parity_known: %d\n", avs_is_parity_known(avs->vi));
>   
> -    /* The following typically only works when assumetff (-bff) and
> -     * assumefieldbased is used in-script. Additional
> -     * logic using GetParity() could deliver more accurate results
> -     * but also decodes a frame which we want to avoid. */
>       st->codecpar->field_order = AV_FIELD_UNKNOWN;
> -    if (avs_is_field_based(avs->vi)) {
> +    /* separatefields makes field_based==true, weave makes field_based==false
> +     * only non field_based clips can therefore be interlaced */
> +    av_log(s, AV_LOG_TRACE, "avs_is_field_based: %d\n", avs_is_field_based(avs->vi));
> +    if (avs_is_field_based(avs->vi) == 0) {
>           if (avs_is_tff(avs->vi)) {
>               st->codecpar->field_order = AV_FIELD_TT;
>           }
> 

The change seems okay, but the comment and commit message need to
explain what's going on better, because the confusion that's erupted
over this seems to derive from the rather poor way the AviSynth
documentation describes the difference between field-based and 
frame-based clips, and how to access this information through the API. 
After having read through all of this a bit more, the situation appears to
be as follows:

AviSynth works on Frame-based video.  Full stop.  'Frame-based',
despite the name, is not a synonym for 'progressive', merely that
it's being processed as a single frame rather than as a half-height
field.  This is the single point from which all the rest of this
got confused.

Using SeparateFields(), you can break the frame into its constituent
half-height fields so that certain filters which don't like processing
interlaced footage can filter the fields as a sort of 'fake
progressive', before using Weave() to combine the fields again into
a singular full height frame.  That's its intention, rather than to
signal to a client program that something is interlaced. The
AssumeFieldBased() function and avs_is_field_based API check are
there to allow other functions to understand this situation
where the fields have been broken apart (or to override the
field-based setting in cases that it didn't get set properly).

The confusion seems to have arisen from mistaking these as ways
that AviSynth indicates something is interlaced or not, and it
isn't.  For that purpose, you have to look at frame properties,
something that was introduced in AviSynth+ as a flag that a source 
plugin can set to indicate whether the frame is interlaced (either
tff or bff) or progressive.  Adding frame property detection to
the demuxer here would require larger changes that need to be
guarded from 2.6, but it would accomplish the goal I *think*
this is ultimately intended to address.

The in-code comment should probably be something closer to:

/* AviSynth works on frame-based video by default, which can
/* be either progressive or interlaced. Some filters can break
/* frames into half-height fields, at which point it considers
/* the clip to be field-based (avs_is_field_based can be used
/* to check for this situation).

/* To properly detect the field order of a typical video clip,
/* the frame needs to have been weaved back together already,
/* so avs_is_field_based should actually report 'false' when
/* checked. */

Am 2021-05-27 22:38, Stephen Hutchinson wrote:

> The change seems okay, but the comment and commit message need to
> explain what's going on better, because the confusion that's erupted
> over this seems to derive from the rather poor way the AviSynth
> documentation describes the difference between field-based and
> frame-based clips, and how to access this information through the API.
> After having read through all of this a bit more, the situation
> appears to
> be as follows:
> 
> AviSynth works on Frame-based video.  Full stop.  'Frame-based',
> despite the name, is not a synonym for 'progressive', merely that
> it's being processed as a single frame rather than as a half-height
> field.  This is the single point from which all the rest of this
> got confused.
> 
> Using SeparateFields(), you can break the frame into its constituent
> half-height fields so that certain filters which don't like processing
> interlaced footage can filter the fields as a sort of 'fake
> progressive', before using Weave() to combine the fields again into
> a singular full height frame.  That's its intention, rather than to
> signal to a client program that something is interlaced. The
> AssumeFieldBased() function and avs_is_field_based API check are
> there to allow other functions to understand this situation
> where the fields have been broken apart (or to override the
> field-based setting in cases that it didn't get set properly).
> 
> The confusion seems to have arisen from mistaking these as ways
> that AviSynth indicates something is interlaced or not, and it
> isn't.  For that purpose, you have to look at frame properties,
> something that was introduced in AviSynth+ as a flag that a source
> plugin can set to indicate whether the frame is interlaced (either
> tff or bff) or progressive.  Adding frame property detection to
> the demuxer here would require larger changes that need to be
> guarded from 2.6, but it would accomplish the goal I *think*
> this is ultimately intended to address.
> 
> The in-code comment should probably be something closer to:
> 
> /* AviSynth works on frame-based video by default, which can
> /* be either progressive or interlaced. Some filters can break
> /* frames into half-height fields, at which point it considers
> /* the clip to be field-based (avs_is_field_based can be used
> /* to check for this situation).
> 
> /* To properly detect the field order of a typical video clip,
> /* the frame needs to have been weaved back together already,
> /* so avs_is_field_based should actually report 'false' when
> /* checked. */

Yeah i think a huge part of all the confusion is that all existing 
descriptions are written from an "inside avisynth" perspective, while 
from feeling most readers see it from the "recieving applications 
perspective". From this perspective, it is kind of guaranteed that 
field_based clips have to be threated as progressive because is not 
really a chance that the delivered frames are interlaced - except a 
filter did not set the value correctly.
Checking the Frame property as you suggest could make it for sure better 
but honestly i don't see a need for it because env->field- and frame 
based should be set correctly by the filters. It would be a shame if the 
demuxer would have to actually decode a frame to get this propety 
correctly - also it could potentially take many minutes for the first 
frame to be decoded when the script does lots of smart stuff and the 
resolution and bit depth is very high. Last but not least, you could 
potentially decode frame 0 while the script doesn't even need frame 0 
because it starts at another one.

However, leaving all this aside, i also hope my stuff makes the 
situation better than it was before and of course
i am ok with your comment version, have nothing to add.
You want me to change it and re-send the patch (i always struggle in how 
exactly to do that, you want me to work from my current fork where the 
commit i sent is included , alter it and commit again - or you want me 
to start from scratch and send a totally new patch?)

Thanks a lot Steve
-emcodem