happycube / ld-decode

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

Improve ld-disk-stacker handling of padded field metadata #863

Closed ripose-jp closed 1 year ago

ripose-jp commented 1 year ago

This fixes a bug where corrupt field metadata from the first input is propagated to the output metadata of ld-disc-stacker. Essentially it assumes that ld-discmap was run with -u so that all corrupt fields get marked with "pad": true. It then takes all fields that have "pad": true and replaces them with field metadata from other fields where "pad": false if such metadata exists. This is necessary since corrupt metadata tends to mess up colors when running ld-chroma-decoder.

There's a lot of code duplication in this commit which I'm not particularly happy about, but I can't really think of a cleaner way to do it. If you have any ideas, I'm all ears.

atsampson commented 1 year ago

Looks OK to me (although I haven't tested it). You could get rid of the duplication by making correctMetaData take a bool flag to say whether to correct the first or second field, then call it twice from process?

ripose-jp commented 1 year ago

Not a bad idea. I updated the code to use a template and if constexpr so the compiler is guaranteed to evaluate the if statements. I'm happy with this solution.

ripose-jp commented 1 year ago

I've updated to commit after finding a few bugs and improvements I could add to it.

The first thing I discovered is that switching out padded metadata with good metadata doesn't necessarily fix colors. Rather colors get messed up in most (~all?~ No, one grayscale frame made it to the output) cases because of phase skips. Swapping out padded metadata alone has a 50% chance of fixing colors. This is because phases go from 1 > 2 > 3 > 4 > 1 > ... and when field metadata is swapped out there's a 50% chance it has the same field phase ID as the metadata it's swapping with. On the off chance the phase IDs are different, the frame is colored orange and blue. I found I could fix this by looking for the first non-padded frame, and then setting the phase IDs sequentially based on the assumption that that phase ID is correct. This produces good colors for all frames so far in my testing.

In the old version of this commit, dropout metadata was chucked and the sequence number wasn't guaranteed to be correct when swapping out padded field metadata. I've fixed the issue by transferring over the good metadata to the swapped metadata.

EDIT Interestingly, one grayscale frame made it to the output. This frame appears in the stacked capture regardless which order files are processed in. This means it isn't a metadata issue like the others. I'll consider it beyond scope unless I can figure out a good way to deal with it. The only difference I've found so far seems to be in the non-visible portion of the data.

Color Grayscale
good frame bad frame

EDIT 2 Turns out some junk frames made it past ld-discmap -u without being marked as pad, though it looks like they were deleted? Makes sense since a black frame getting averaged into the output could cause this.

ripose-jp commented 1 year ago

Turns out grayscale frames can occur when averaging the results of two frames together. Essentially what would happen is, one of my three sources would be marked pad for a frame, so ld-disc-stacker would fall back to averaging pixels. When this happens, it might destroy the color burst leading to a grayscale frame. Maybe this happens because field lines are shifted a few microseconds between the two sources. This is just speculation though. The way I've fixed it is by only taking the first available pixel when less than 3 sources are available. I won't be contributing back this solution since it's harmful to the way ld-disc-stacker works, but it's good for my purposes. Maybe ld-disc-stacker could fix this by only averaging pixels outside the colorburst region?