| Message ID | 3af35fff5c1a757447a3bcf1cc994324@ffastrans.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] Populate field order returned by avs script, Trac Ticket 8757 | expand |
| 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 |
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.
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.
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. */
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
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?
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 --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:
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(+)