Crashes with odd image height in Stereo 3D

[olafmatt]

I have a bunch of files shot with a DSLR on a slider and converted to 2k CineForm 3D files. Due to the sensor size the 2k frames are 2048x1363. So this is an odd image height (not a multiple of two).

The decoder returns a Stereo 3D image when set to VIDEO_SELECT_BOTH_EYES and STEREO3D_TYPE_DEFAULT that is 2048x2728 (i.e. not 2048x2726 as one would expect) and has a black line (single pixel height) below each of the views/eyes.

Switching the Stereo 3D types to STEREO3D_TYPE_STACKED or STEREO3D_TYPE_FIELDS crashes the decoder. All other types work correctly (as long as I make the output buffer large enough to hold an extra scanline).

[dnewman-gpsw]

Does everything work if you crop the source to 2048x1362? Formats like STEREO3D_TYPE_STACKED or STEREO3D_TYPE_FIELDS can't work with odd sizes. STACKED would present both frames in under-over scaled to fit the original frame size, which would force left and right views to be none integer sizes. Interlaced would have left and right with differing numbers of scan lines.

[olafmatt]

Files with even number of scanlines (per view/eye) work fine. I do understand that some of the modes don't really make sense with odd frame heights. But then they should not crash and those that do work and do make sense (like the "normal" top/bottom aka. STEREO3D_TYPE_DEFAULT) should just work normally without any extra black scanlines between the two eyes.
I can provide test files, if you want to give it a try yourself.

[dnewman-gpsw]

Also the code is now here for others to help solve. Best solution would be to automatically crop incompatible input so that there is no ambiguity over what will work with a encoded file.

[olafmatt]

Yes, I'm slowly working my way though the code, trying to fix things... But I don't like the idea of "just don't encode frames with an odd height". The encoder should either refuse to do so or the decoder should handle it correctly (or give an error, but NOT crash).

[olafmatt]

After more testing I found the following: The black scanline does not just affect Stereo 3D files but also normal files if they have an odd height. Just that with Stereo 3D in top/bottom view the black scanline between the views is easier to spot.

What is actually happening is that for images with an odd image height the last scanline gets displayed black. This only happens when the TAG_DECODE_CURVE metadata is set to the same value as the encoding curve specified in the file (i.e. no curve conversion required).
My sample file has CFHD_CURVE_GAMMA_2pt2 set as encode and decode curve. When I request CFHD_CURVE_GAMMA_2pt2 as decode curve the last scanline is black. Then I tried switching the decode curve to CFHD_CURVE_LOGC. Now the last scanline gets decoded properly. Switching back to CFHD_CURVE_GAMMA_2pt2 now gives a last scanline that still contains the LogC image data. The same is true for any other encode/decode curves specified in the file.
Conclusion: When decoding to the same curve as encoded (i.e. no change) the last scanline doesn't get rendered (i.e. written to the output buffer) at all.
This ONLY applies to RGB encoded files, YUV is fine. My output pixel format is CFHD_PIXEL_FORMAT_RG48.

[dnewman-gpsw]

Odd scanline numbers from RGB encodes should be supported. That is a bug.

[olafmatt]

You have an idea / hint where I should be looking in the code? I was trying to find where curves get changed, but somehow always ended up in bayer.c which is probably not where I should be looking.

[dnewman-gpsw]

RAW (Bayer) was where all the curves and ActiveMetadata was developed first, so stepping into bayer.c makes sense. However, the failure is likely in the last scanline of the wavelet. Different wavelet code may be used if ActiveMetadata image development is active. So having a mismatch in encode and decode curve will likely use a different inverse wavelet, one that handles the odd scanline, and one that doesn't. Look at the last wavelet used.

[olafmatt]

Found something. In wavelet.c most of the TranformInverseXXX() functions do this:

//DAN20050606: Added to fix issue with heights that are not divisible by eight
last_display_row = info->height/2;

However, TransformInverseRGB444ToRGB32() does it like this:

//DAN20050606: Added to fix issue with heights that are not divisible by eight
last_display_row = (info->height+1)/2; // DAN20090215 -- fix for odd display lines.

So for a test I used CFHD_PIXEL_FORMAT_B64A as output format instead of CFHD_PIXEL_FORMAT_RG48 and now the last scanline gets rendered correctly.

[dnewman-gpsw]

Yes, the "DAN20090215" note looks like is might be useful on other pixel format optimized wavelet transforms (TranformInverseXXX()).

Try: last_display_row = (info->height+1)/2;

[olafmatt]

That seems to fix it... I just replaced all occurances with the version that is rounding up instead of truncating.

However, just below the "DAN20090215" fix is the following:

odd_display_lines = info->height & 1;

And then further down:

	if(odd_display_lines)
	{
		if(row == last_display_row - do_edge_row - 1)
		{
			strip.height = 1;
		}
	}

The other verions of TranformInverseXXX() don't do this either. So I'm not sure my "fix" is the full story or I'm just lucky that I don't run into more/other problems now.

[olafmatt]

On closer inspection it seems that the odd_display_lines variable makes sure that on the last iteration of the loop we don't generate two output rows but just one. So I guess just applying my change means it will now write past the end of the buffer.

[dnewman-gpsw]

Yes. You will need to prevent writing outside of the buffer.

[olafmatt]

Added that check for the last single row to all functions. Will do more testing with that version, but for now here's my modified wavelet.c.

wavelet.zip

[dnewman-gpsw]

Changes to TransformInverseRGB444ToRGB48 aren't good. With TestCFHD set the resolution to 1081 and run the decoder test -D, it will crash with those changes. I'm looking at the benefits of the other modifications.