happycube / ld-decode

Software defined LaserDisc decoder
GNU General Public License v3.0
304 stars 80 forks source link

Improve Comb::filterIQ's filter #513

Closed atsampson closed 2 years ago

atsampson commented 4 years ago

Comb::filterIQ is the IQ post-filter in the NTSC decoder - in a conventional decoder, it would filter off aliases in the I/Q signals after demodulation.

At present, it doesn't actually do anything because of a bug in Comb::decodeFrame: in both places where it's called, it's applied to currentFrameBuffer.yiqBuffer rather than tempYiqBuffer. If you fix that, it doesn't work very well (the filter delay compensation needs adjusting, and its filters aren't linear-time anyway).

The NTSC decoder seems to work perfectly well without the postfilter - I guess the demodulator effectively works like a Tayloe detector rather than a conventional mixer. So should I fix the bug above but have it turned off by default, or should I just remove filterIQ entirely?

atsampson commented 4 years ago

Looking at the old motion detection code, that had the same problem:

        splitIQ(&currentFrameBuffer);

        tempYiqBuffer = currentFrameBuffer.yiqBuffer;

        adjustY(&currentFrameBuffer, tempYiqBuffer);
        if (configuration.colorlpf) filterIQ(currentFrameBuffer.yiqBuffer); // <-- here
        doYNR(tempYiqBuffer);
        doCNR(tempYiqBuffer);

        opticalFlow.denseOpticalFlow(currentFrameBuffer.yiqBuffer, currentFrameBuffer.kValues); // <-- and here

splitIQ leaves the baseband signal in y, so denseOpticalFlow was seeing the baseband signal rather than Y. That may be another reason why it never worked very well...

simoninns commented 4 years ago

I think this is a question for @happycube - but my vote would be 'remove'

Gamnn commented 4 years ago

It's functional in master now.

Off: frame_ntsc_comb_47505

On: frame_ntsc_comb_47505pf

simoninns commented 4 years ago

So, what are we actually saying here... the issue is solved and should be closed?

Gamnn commented 4 years ago

Perhaps not entirely. It works in ld-analyse, but I don't see an option for it in ld-chroma-decoder.

atsampson commented 4 years ago

I've made it "work" in that you can enable it now, but the filter isn't useful at the moment - the delay compensation isn't correct, and it has different delays at different frequencies (if you try it on a colour image you'll see it shifts different colours different amounts to the right). We could replace it with a linear-time FIR filter, or we could do the "filtfilt" trick (design an IIR filter with half the required amplitude, then run the data through it twice, forwards then backwards, so the delay cancels out).

simoninns commented 4 years ago

I was trying to work out if the issue should be closed or left open (but with some indication of why) :) You guys are not making this any easier! Is a) the issue closed or b) not closed (please add what needs to be done) ;)

atsampson commented 4 years ago

I think I've more or less convinced myself that fixing it (by generating a new filter) is worthwhile. I'll retitle the issue...