CrendKing / avisynth_filter

DirectShow filters that put AviSynth and VapourSynth into video playing
MIT License
108 stars 8 forks source link

Interlaced flag (?) after AVSF #59

Closed chainikdn closed 3 years ago

chainikdn commented 3 years ago

MPC-BE, EVR-CP, LAV decoder, sw decoder, no SVP AVI file containing progressive Xvid 704x528 29.97fps stream. https://drive.google.com/file/d/1qTV5lPS-uWZgAIDRjSN5eeleotB69teo/view?usp=sharing

Playing w/o AVSF: first two lines in renderer stats:

Frame rate: 29.980 (...) Frame type: Progressive

Playing with AVSF:

Frame rate: 59.980 (...) Frame type: Interlaced : Bottom fields first

Probably related to #53 ?

Other user said it started after 0.9.4.

CrendKing commented 3 years ago

Here's the thing. MPC-BE has this function in IPinHook.cpp:

static HRESULT STDMETHODCALLTYPE ReceiveMineI(IMemInputPinC * This, IMediaSample *pSample)
{
    if (pSample) {
        // Get frame type
        if (CComQIPtr<IMediaSample2> pMS2 = pSample) {
            AM_SAMPLE2_PROPERTIES props;
            if (SUCCEEDED(pMS2->GetProperties(sizeof(props), (BYTE*)&props))) {
                g_nFrameType = PICT_BOTTOM_FIELD;
                if (props.dwTypeSpecificFlags & AM_VIDEO_FLAG_WEAVE) {
                    g_nFrameType = PICT_FRAME;
                } else if (props.dwTypeSpecificFlags & AM_VIDEO_FLAG_FIELD1FIRST) {
                    g_nFrameType = PICT_TOP_FIELD;
                }
            }
        }
    }

    return ReceiveOrg(This, pSample);
}

So it defaults to bottom field interlaced unless it finds AM_VIDEO_FLAG_WEAVE in the AM_SAMPLE2_PROPERTIES::dwTypeSpecificFlags field. When AVSF receives the input sample from upstream (which indeed has AM_VIDEO_FLAG_WEAVE), it never cared about dwTypeSpecificFlags. So on the output side, that flags got lost.

On the other hand, MPC-HC's approach to detect interlaced is checking VIDEOINFOHEADER2::dwInterlaceFlags, which we do carry over to the output side. So the result stats in MPC-HC is correct.

I'm leaning towards not fixing and hoping MPC-BE one day switches to use dwInterlaceFlags. Because saving and restoring per-sample properties is time consuming, while it seems to be purely cosmetic and not causing any rendering issue. If there is actual damage of not setting the properties, please let me know and I'll reconsider.

chainikdn commented 3 years ago

Sounds odd. If you have some flag on input, why not keeping it on output? :/ Can you guarantee that MPC-BE doesn't do some kind of deinterlacing then? Can you guarantee than some other player won't do it?

CrendKing commented 3 years ago

I think the dwTypeSpecificFlags field is redundant information. You already have the interlaced flags in media type. If the sample has exactly same information, why exist in both places? If they differ, which is the source of truth? The doc says "Type-specific flags", not "Sample-specific flags". So all I'm saying is I'm picking the media type as the source of truth and ignoring the other, because media type applies to all samples until type change, which is more performant. I don't know why dwTypeSpecificFlags exists.

I can't guarantee anything that I don't control. If a player has some unwanted behavior, we can discuss and figure out solution. If not, I think status-quo is acceptable atm.

chainikdn commented 3 years ago

I think the dwTypeSpecificFlags field is redundant information

so what? you can argue with Microsoft about it ;) but do you really want to decide which one must someone use? is it forbidden by the docs to use dwTypeSpecificFlags? I believe no.

If a player has some unwanted behavior, we can discuss and figure out solution

I really have enough tasks other than searching for "unwanted behaivor" in multiple players.


What is the problem exactly with copying one dword to the output sample?

CrendKing commented 3 years ago

Fine: https://github.com/CrendKing/avisynth_filter/actions/runs/1043412908

CrendKing commented 3 years ago

In released v1.2.0, I made some changes to the implementation above. See the release note for detail. Downstreams that still uses AVS+ 3.5 such as SVP, all videos will be progressive seen from AVSF downstream. If AVS+ 3.6 is used, things will be correct in both variants thanks to frame properties.

chainikdn commented 3 years ago

I'm afraid a memory leak appeared after this commit. At least build from 19/07 is already "infected". SVP is off, MPC-HC, start playback, seek several times, switch to the next file, seek again --> observe very slow seeking plus +100-200 MB memory usage with every seek. (I'm watching Vikings season 6 1080p) Important - doesn't happen with the first opened file.

CrendKing commented 3 years ago

Yes. The bug is introduced here. The lambda captures this, which is the entire FrameHandler, while being static. So even switching video (which re-initialize the whole filter) will not destroy that instance.

https://github.com/CrendKing/avisynth_filter/commit/b8719cd5e49cbfd2818ed587de84af28aa338e96 should fix this.

Fun fact, I replicated the bug in a separate project with AddressSanitizer enabled. It instantly caught the bug. Unfortunately as a dll module I can't just enable ASAN without touching the hosting player process.