Consolidated all the cameras that only do the 3x3_1X crop_rec option into a "basic" group that shares much of the code. This simplifies the code and makes it easier to add other cameras. This may be overstepping the intent of this pull request to just add the 650D but it seemed like a good thing to do.
Tested on the EOSM, 700D and 5D3.113
Can you also look into this issue? (raw_diag OB zones test)
IIRC these sensors have the same resolution, so they may need the same fix (the skip offset overrides may have to be disabled or tweaked for these models).
@danne and I have been looking into this. We know that on the EOSM, 100D, 700D and most likely also on the 650D there's a black bar a the top of the frame when using crop_rec 3x3_1X at full resolution.
We'll submit a fix if we are successful at figuring it out.
Simply don't call calc_skip_offsets/raw_set_geometry for these cameras.
Yes, that works (of course, I never doubted you). Now I've got to figure out a way to do it gracefully and understand why Adobe Camera Raw displays the image as 1736x1147 when it is really 1736x688. This didn't happen before.
There isn't any reason to define the skip_* variables in this case, is there? Just making sure I'm not leaving out too much here.
Never mind about that ACR comment -- that has to do with the way the DNGs are processed and isn't related to this issue.
There is some change in the way crop_rec behaves. Calling calc_skip_offsets/raw_set_geometry will result in the black bar and the Auto Preview defaults to Framing but not calling calc_skip_offsets/raw_set_geometry will not show the black bar and Auto Preview defaults to Real-time which for crop_rec is stretched vertically.
I also answered my question -- don't need to define the skip_* variables.
I'd like to do some more tests before updating the pull request.
What I'm wondering: without this change, on any of the affected models, do you get correct black level in MLV or in DNG EXIF, or are you using some workarounds in post to fix it to 2048 manually?
The area on the left of the "blue box" is used for computing the black level. If half of this covers real image data, I expect this to throw off the calculations and also the internal checks (so if the scene has big brightness variations on the left side, e.g. top bright and bottom dark, you should not even be able to start recording).
Do you have a silent DNG around (from any of the small models with crop_rec) so I can check this in QEMU? It should be some real image, not just an uniform frame as the ones used for focus pixels.
That's exactly what I did! Still, I want to test this on all the cameras I've got available before committing the change.
I'm working today but should be able to check this out in the next few days. Hunting down down the commit where this first appeared seems to be something worth looking into.
Alright, let's wait for some samples now (as I have yet to see a valid crop_rec/lossless DNG from 650D).
I have yet to create a valid crop_rec/lossless silent DNG. I keep getting an mv720 (5x3?) full raw buffer with the silent module. Is that what you want to see or just a DNG extracted from an MLV file?
You mean, even with crop_rec enabled, the silent module saves a squeezed DNG?!
Ok, no black bar and it is confirmed working on 5D3, EOSM, 100D, 700D of course this pull request is for the 650D but it should be safe to assume it works on that camera. Had a strange issue with the EOSM that didn't work with "if (!is_basic)" but it is fine with "if (is_5D3)"
Maybe some older build was on the card? Otherwise, bugs like that (apparently not following the logic from the code) may indicate a bigger problem (such as memory corruption). Hope it was a mistake.
Just tried it again and "if (!is_basic)" or "if (is_5D3)" is working fine on the EOSM. Must have been my mistake. Danne reported that this also works:
Oops--he left out 650D. I like checking for the 5d3, we're checking for a true instead of a false and it seems cleaner to me.
Finally got a valid crop_rec/lossless silent DNG - from the 700D.
Why is the dng image filled 1736x696 but according to rawdiag it shouldn´t(check left)? Also check bottom line at the first rawdiag image
The differences are:
66 vs 72 for skip_left; mlv_lite rounds to 8 pixels => it uses 72 in both cases; silent.mo does not, and you'll see the black bar on the left in that case.
skip_bottom = 4 vs 0, but there's a 8-pixel difference; mlv_lite adjusts height to get WxH multiple of 16 bytes (EDMAC restriction): W=1736*14/8 (multiple of 2 but not of 4), so H must be multiple of 8 pixels.
So... yeah, worth overriding skip_bottom in crop_rec then.
edit: wait a minute, is skip_bottom = 4 even needed? (A: yes, visible in nikfreak's screenshot)
I read about limitations above and check raw_diag images. Then I test and change in raw.c from:
// 650D and EOSM probably need to fit into this// http://www.magiclantern.fm/forum/index.php?topic=16608.msg174241#msg174241#if defined(CONFIG_700D) || defined(CONFIG_100D)skip_top=28;skip_left=72;skip_right=0;skip_bottom=zoom?0:mv1080crop?0:4;#endif
// 650D and EOSM probably need to fit into this// http://www.magiclantern.fm/forum/index.php?topic=16608.msg174241#msg174241#if defined(CONFIG_700D) || defined(CONFIG_100D)skip_top=28;skip_left=72;skip_right=0;skip_bottom=zoom?0:mv1080crop?0:mv720?0:4;#endif
"edit: wait a minute, is skip_bottom = 4 even needed? (A: yes, visible in nikfreak's screenshot)"
My eyes see a perfect raw_diag(and a 1728x698 dng file) without skip_bottom = 4 in crop_rec, I am told about limitations, and I am pointed to Nikfreak´s screenshot which seems to be the reason why we need skip_bottom = 4?
I am lost ;)
*Update the file is 1728x698 not 1736x698(rounding?)
*Default seems to become 1736x696 instead of 1736x688(better with more resolution?)
But yes, whte border at the bottom. Doesn´t explain why we have to limit crop_rec to 1736x688 since filling the image up to 1728x698? I only want to understand.
We don't have to; we need the bottom limit for non-crop 720p.
Were these DNGs in crop mode or non-crop edit: figured it out
1736x688 is valid, 1728x698 is valid, 1736x698 is not (EDMAC restrictions). Try removing them and you'll see the camera crashing (EDMAC controller not knowing where to stop writing).
In : 1736*14//8*688 % 16
In : 1728*14//8*698 % 16
In : 1736*14//8*698 % 16
More info: edmac-memcpy.c - edmac_copy_rectangle_cbr_start
Adding crop_rec to this family in raw.c?
/* params useful for hardcoding buffer sizes, according to video mode */intmv=is_movie_mode();intmv640=mv&&video_mode_resolution==2;intmv720=mv&&video_mode_resolution==1;intmv1080=mv&&video_mode_resolution==0;intmv1080crop=mv&&video_mode_resolution==0&&video_mode_crop;intmv640crop=mv&&video_mode_resolution==2&&video_mode_crop;intzoom=lv_dispsize>1;
Thanks for edmac explanation.
I just revisited this comment in the code and finally got what @nikfreak was telling me.
// 650D and EOSM probably need to fit into this// http://www.magiclantern.fm/forum/index.php?topic=16608.msg174241#msg174241
Need to do some testing but this seems to be the right branch for these sort of experiments.
Should we continue here or do this on a separate pull request?
Let's try on a new PR on the same branch.
Wouldn´t mind if we get in a setting for crop_rec in there...
I'd like it if this could be merged before starting on those other changes because this pull request is now affecting several cameras. The current crop_rec_4k branch has that black bar on EOSM/100D/700D crop_rec 3x3_1X footage.
I have a feeling the 650D might need a few more register tweaks (but can't tell for sure before seeing a DNG taken in this mode). So far, no 650D user submitted any samples...
Will merge it in this state, but will not enable a build yet - the lossless compression needs more investigation (not straightforward).