Fix/code review safe fixes#1044
Merged
Merged
Conversation
FilterParams_PAL_lowband['video_bpf_order'] was set to 13_000_000, which was a copy-paste of the video_bpf_high value on the line above. When --lowband is used with a PAL source, computevideofilters() passes this order to scipy.signal.butter, which then tries to build a transfer function of degree ~26M via repeated convolution. The result is an effective hang / memory exhaustion. Set the order to 2 (matching FilterParams_PAL); FilterParams_NTSC_lowband similarly leaves order untouched at the base value.
seek_getframenr() returns (fnum, startfield, readloc) on every path
except the early-CLV branch, which returns only (None, startfield).
Its caller, seek(), unpacks three values:
fnr, curfield, readloc = self.seek_getframenr(curfield)
so on an early-CLV disc the seek crashes with
"ValueError: not enough values to unpack (expected 3, got 2)"
instead of producing the intended "Cannot seek in early CLV disks"
error.
Match the other return paths by appending None for readloc.
getpulses(do_retry=True) takes a do_retry argument and the existing recursive call already passes do_retry=False, signalling clear intent to limit the retry to one round. But the guard around the recursion only checked `not self.fields_written`, ignoring do_retry entirely. On the first field, if findpulses() returns empty even after the ire0 recalibration (blank input, heavily corrupted capture, wrong format), getpulses() recurses without bound and dies with RecursionError instead of returning gracefully. Include do_retry in the guard so the recursion stops after one retry.
The PAL_V4300D_NotchFilter path in demodblock_cpu zeroes anomalous bins by writing into indata_fft in place. When demodblock_cpu is called with a precomputed fftdata (the common path from DemodCache.worker), indata_fft aliases the cached array that the cache stores back into the block. AGC and MTF retries re-invoke demodblock_cpu against the same block, so each pass zeroes more bins of the already-modified spectrum, progressively corrupting the cached FFT. Copy the array before zeroing so the cache stays clean and each re-decode starts from the original spectrum. Only the V4300D branch pays the copy cost; other paths are unaffected.
hz_to_output_array converts Hz to uint16 picture samples via np.uint16(x), which truncates toward zero. The scalar path in Field.hz_to_output adds 0.5 before the cast so it rounds to nearest. Field.hz_to_output dispatches arrays through hz_to_output_array, so the disagreement applied to every pixel in the decoded picture: each output sample was biased roughly half an LSB low, and differed from the scalar/JSON-reported values by up to a full LSB. Add the +0.5 so the array path rounds the same way.
Replace the np.angle + ediff1d + np.unwrap + re-wrap-clamp implementation of unwrap_hilbert() with a conjugate-product FM discriminator that takes the per-sample phase increment directly as arg(z[n] * conj(z[n-1])). - Numerically equivalent to the previous angle-difference + unwrap method on well-behaved data (verified to <1e-6 Hz on clean and noisy video- and audio-band FM signals). - Output is bounded to [0, freq_hz) by construction, so the iterative 'fixangles' while-loop clamps are no longer needed. - Fully numba-jittable: removes the np.unwrap() path that the numba<0.59 deprecation branch had to run outside numba, unifying the two branches. - One arctan2 pass instead of arctan2 + unwrap + clamp passes (~25% faster in pure numpy). No change to decoded output on good data; this is a robustness/perf cleanup.
PAL now builds the pre-demodulation RF video filter as a high-pass (low edge) cascaded with a low-pass (high edge) rather than a single Butterworth band-pass, so the two skirts can be ordered independently. - Low edge unchanged (order 2 @ 2.3 MHz): the lower chroma sideband (~2.67 MHz) and its group delay are preserved. - High edge sharpened and raised (order 3 @ 14.0 MHz, was order 2 @ 13.5 MHz): the upper chroma sideband (~12.3 MHz) stays flatter (-0.6 vs -1.5 dB) and HF noise plus the folded upper-J2 product (~16 MHz) are rejected better (~2 dB at 16 MHz, ~8.5 dB at 18 MHz), narrowing the noise bandwidth into the demodulator. NTSC is unchanged and continues to use the shared single Butterworth band-pass.
Adds an all-pass group-delay equaliser to the PAL post-demodulation video path and widens the PAL video low-pass filter to 5.8 MHz. - build_groupdelay_equalizer() synthesises a unit-magnitude FFT-domain all-pass whose group delay is (IEC 60856 9.1.6 target - actual LPF), so the LPF * equaliser cascade reproduces the spec group-delay curve across the chroma band (10/35/85/135/200 ns at 2/3/4/4.43/4.8 MHz). The disc is pre-distorted against that curve and the Butterworth LPF alone undershoots it, which smears colour. De-emphasis is excluded from the basis (it is cancelled end-to-end by the disc's inverse pre-emphasis). Applied only to the FVideo output path; magnitude is unchanged (pure all-pass). - video_lpf_freq 5.2 -> 5.8 MHz recovers recorded luma out to the 5.8 MHz VITS multiburst; the equaliser auto-refits (its taper tracks the cut-off) so the cascade still matches 9.1.6. PAL only; NTSC is unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
Description
Various fixes for corner cases not exercised by test cases
Motivation
Make code more robust and return useful error messages .
Related Issues
none
Changes Made
Testing
pytest --output-on-failure)Screenshots (if applicable)
none
Additional Notes
none