diff mbox series

[FFmpeg-devel] Populate field order returned by avs script, Trac Ticket 8757

Message ID 3af35fff5c1a757447a3bcf1cc994324@ffastrans.com
State New
Headers show
Series [FFmpeg-devel] Populate field order returned by avs script, Trac Ticket 8757
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

emcodem@ffastrans.com Jan. 17, 2021, 12:26 a.m. UTC
The purpose of this is to tell ffmpeg which field order the returned 
clip of the avisynth .avs script decided to finally deliver, which is 
handy in an automated environment.
Interlacing is generally a very hard topic in avisynth, the huge comment 
is neccessary to explain my reasons.
Additionally to the comment in the patch, i can tell that i set unknown 
instead of progressive for backward compatibility. (i was struggeling 
with this, maybe i should have even left it unset in case it is not 
clearly tff or bff interlaced.

    /*  Set interlacing type. 
http://avisynth.nl/index.php/Interlaced_fieldbased
     *   The following typically only works when assumetff (-bff) and 
assumefieldbased is used in the avs.
     *   This is because most avisynth source plugins do not set the 
parity info in the clip properties.
     *   We could use GetParity() to be more accurate but it decodes a 
frame which is
     *   expensive and can potentially lead to unforeseen behaviour when 
seeking later.
     *   In case Parity is not known, we still cannot guarantee that
     */

Tests with different avisynth source plugins, e.g. directshowsource, 
ffms2, mpeg2source delivered the expected result.
Make FATE passed.
Subject: [PATCH] Populate field order returned by avs script, Trac Ticket 8757

---
 libavformat/avisynth.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Stephen Hutchinson Jan. 17, 2021, 8:02 a.m. UTC | #1
On 1/16/21 7:26 PM, emcodem@ffastrans.com wrote:
> The purpose of this is to tell ffmpeg which field order the returned 
> clip of the avisynth .avs script decided to finally deliver, which is 
> handy in an automated environment.
> Interlacing is generally a very hard topic in avisynth, the huge comment 
> is neccessary to explain my reasons.
> Additionally to the comment in the patch, i can tell that i set unknown 
> instead of progressive for backward compatibility. (i was struggeling 
> with this, maybe i should have even left it unset in case it is not 
> clearly tff or bff interlaced.
> 
>     /*  Set interlacing type. 
> http://avisynth.nl/index.php/Interlaced_fieldbased
>      *   The following typically only works when assumetff (-bff) and 
> assumefieldbased is used in the avs.
>      *   This is because most avisynth source plugins do not set the 
> parity info in the clip properties.
>      *   We could use GetParity() to be more accurate but it decodes a 
> frame which is
>      *   expensive and can potentially lead to unforeseen behaviour when 
> seeking later.
>      *   In case Parity is not known, we still cannot guarantee that
>      */
Going into detail about GetParity wouldn't be necessary if it's not
used (and there aren't any other invoke-parsed functions aside from
checking with Import() whether the script actually returns a clip),
so the comment could be shortened.  Also, since this is the avisynth
demuxer, 'in the avs' would be better rendered as 'in the script',
and simply refer to 'source plugins' rather than 'avisynth source
plugins'.

Comment bikeshedding aside, LGTM, but the avs_is* API usage added here
needs to be reflected in the AVSC_DECLARE_FUNC and LOAD_AVS_FUNC blocks.
If those parts of the API are present in 2.5, the LOAD_AVS_FUNC can be a
0. If they were added in 2.6 (or Plus, but I know these would have to be
from classic AviSynth), then it should be 1.
emcodem@ffastrans.com Jan. 17, 2021, 6:23 p.m. UTC | #2
On 2021-01-17 09:02, wrote Stephen Hutchinson:
> Going into detail about GetParity wouldn't be necessary if it's not
> used (and there aren't any other invoke-parsed functions aside from
> checking with Import() whether the script actually returns a clip),
> so the comment could be shortened.  Also, since this is the avisynth
> demuxer, 'in the avs' would be better rendered as 'in the script',
> and simply refer to 'source plugins' rather than 'avisynth source
> plugins'.
> 
> Comment bikeshedding aside, LGTM, but the avs_is* API usage added here
> needs to be reflected in the AVSC_DECLARE_FUNC and LOAD_AVS_FUNC 
> blocks.
> If those parts of the API are present in 2.5, the LOAD_AVS_FUNC can be 
> a
> 0. If they were added in 2.6 (or Plus, but I know these would have to 
> be
> from classic AviSynth), then it should be 1.

Thanks for the very quick reply, i shortened the comment but as the 
whole comment is basically just there to explain why i decided to not 
involve getparity, i believe it is worth to mention in order to save 
some time for possible future committers.

What i am not able to do is to add the used convenience functions 
avs_is_tff and bff to AVSC_DECLARE_FUNC and LOAD_AVS_FUNC, it refuses to 
compile when i do so. IMHO this is because it is just convenience 
functions that's function body is defined in the linked avisynth_c.h 
file instead of being exported by the avisynth api lib.
The existing code in avisynth.c also uses such convenience functions 
without adding them to the declaration, examples:
avs_has_video, line 524
avs_is_clip, line 571

Also, i found it safe to use the convenience functions avs_is_tff and 
bff because the minimum required version is 2.6 and Plus or above, so i 
hoped the functions will be always available.
Stephen Hutchinson Jan. 21, 2021, 1:10 p.m. UTC | #3
On 1/17/21 1:23 PM, emcodem@ffastrans.com wrote:
> On 2021-01-17 09:02, wrote Stephen Hutchinson:
>> Comment bikeshedding aside, LGTM, but the avs_is* API usage added here
>> needs to be reflected in the AVSC_DECLARE_FUNC and LOAD_AVS_FUNC blocks.
>> If those parts of the API are present in 2.5, the LOAD_AVS_FUNC can be a
>> 0. If they were added in 2.6 (or Plus, but I know these would have to be
>> from classic AviSynth), then it should be 1.
> 
> What i am not able to do is to add the used convenience functions 
> avs_is_tff and bff to AVSC_DECLARE_FUNC and LOAD_AVS_FUNC, it refuses to 
> compile when i do so. IMHO this is because it is just convenience 
> functions that's function body is defined in the linked avisynth_c.h 
> file instead of being exported by the avisynth api lib.
> The existing code in avisynth.c also uses such convenience functions 
> without adding them to the declaration, examples:
> avs_has_video, line 524
> avs_is_clip, line 571
> 
> Also, i found it safe to use the convenience functions avs_is_tff and 
> bff because the minimum required version is 2.6 and Plus or above, so i 
> hoped the functions will be always available.

Yeah, never mind about that.  I didn't notice that those are declared
AVSC_INLINE, not AVSC_API, so they don't get used through the dynamic
API loader.

The comment formatting seems to have been messed up in the second
version, though.

/* 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. */
emcodem@ffastrans.com Jan. 21, 2021, 6:08 p.m. UTC | #4
On 2021-01-21 14:10, Stephen Hutchinson wrote:
> Yeah, never mind about that.  I didn't notice that those are declared
> AVSC_INLINE, not AVSC_API, so they don't get used through the dynamic
> API loader.
> 
> The comment formatting seems to have been messed up in the second
> version, though.
> 
> /* 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. */

OK, i have to admit formatting comments is in the top 10 of my greatest 
weaknesses :D
Thanks for your patience and also thanks for telling me directly how the 
formatting is done correctly.
New patch with formatted comment attached
emcodem@ffastrans.com Feb. 1, 2021, 8:44 a.m. UTC | #5
Am 2021-01-21 19:08, schrieb emcodem@ffastrans.com:
> On 2021-01-21 14:10, Stephen Hutchinson wrote:
>> Yeah, never mind about that.  I didn't notice that those are declared
>> AVSC_INLINE, not AVSC_API, so they don't get used through the dynamic
>> API loader.
>> 
>> The comment formatting seems to have been messed up in the second
>> version, though.
>> 
>> /* 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. */
> 
> OK, i have to admit formatting comments is in the top 10 of my
> greatest weaknesses :D
> Thanks for your patience and also thanks for telling me directly how
> the formatting is done correctly.
> New patch with formatted comment attached

Is it OK to ping this?
emcodem@ffastrans.com Feb. 8, 2021, 11:45 a.m. UTC | #6
Am 2021-02-01 09:44, schrieb emcodem@ffastrans.com:
> Am 2021-01-21 19:08, schrieb emcodem@ffastrans.com:
>> On 2021-01-21 14:10, Stephen Hutchinson wrote:
>>> Yeah, never mind about that.  I didn't notice that those are declared
>>> AVSC_INLINE, not AVSC_API, so they don't get used through the dynamic
>>> API loader.
>>> 
>>> The comment formatting seems to have been messed up in the second
>>> version, though.
>>> 
>>> /* 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. */
>> 
>> OK, i have to admit formatting comments is in the top 10 of my
>> greatest weaknesses :D
>> Thanks for your patience and also thanks for telling me directly how
>> the formatting is done correctly.
>> New patch with formatted comment attached
> 
> Is it OK to ping this?

Sorry for Pinging again :-(
I know it's not much that the patch does but the resulting functionality 
from a user perspective can be really useful, especially when using 
programatically calculated avs scripts.
diff mbox series

Patch

diff --git a/libavformat/avisynth.c b/libavformat/avisynth.c
index 2c08ace8db..67348a61d7 100644
--- a/libavformat/avisynth.c
+++ b/libavformat/avisynth.c
@@ -241,6 +241,26 @@  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));
+    
+    st->codecpar->field_order = AV_FIELD_UNKNOWN;
+    if (avs_is_field_based(avs->vi)) {
+    /*  Set interlacing type. http://avisynth.nl/index.php/Interlaced_fieldbased
+    *   The following typically only works when assumetff (-bff) and assumefieldbased is used in the avs.
+    *   This is because most avisynth source plugins do not set the parity info in the clip properties.
+    *   We could use GetParity() to be more accurate but it decodes a frame which is 
+    *   expensive and can potentially lead to unforeseen behaviour when seeking later.
+    *   In case Parity is not known, we still cannot guarantee that 
+    */
+        if (avs_is_tff(avs->vi)) {
+            st->codecpar->field_order = AV_FIELD_TT;
+        }
+        else if (avs_is_bff(avs->vi)) {
+            st->codecpar->field_order = AV_FIELD_BB;
+        }
+    }
+    
     switch (avs->vi->pixel_type) {
     /* 10~16-bit YUV pix_fmts (AviSynth+) */
     case AVS_CS_YUV444P10: