#872 Merged at 7d9155d
Repository
hudson
Branch
crop_rec_4k
Author
  1. Daniel Fort
Reviewers
Description

How about the 650D joining the crop_rec_4k party?

http://www.magiclantern.fm/forum/index.php?topic=7473.msg191490#msg191490

Comments (40)

  1. Daniel Fort author

    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

    1. Daniel Fort author

      @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.

      EOSM example

      black_bar.jpg

      We'll submit a fix if we are successful at figuring it out.

        1. nikfreak

          shall i recheck ob zones / offsets for all modes? I probably missed to do for MV720 - there seems to be only that reason when seeing danne's posted picture. For crop_rec I definitely didn't check it.

          1. nikfreak

            quick test shows: MV720 is spot on (OB zones)

            for crop_rec they are not in expected area (way off as already seen in danne's pic). going to get it done for crop_rec.mo OB-ZONES_mv720.jpg OB-ZONES_croprec.jpg

            1. nikfreak

              any suggestion for defining mv720croprec w/o reinventing the wheel?

              int mv720croprec = mv && video_mode_resolution == 1 && ????;
              
                1. Daniel Fort author

                  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.

                          /* update skip offsets */
                          int skip_left, skip_right, skip_top, skip_bottom;
                  //        calc_skip_offsets(&skip_left, &skip_right, &skip_top, &skip_bottom);
                  //        raw_set_geometry(raw_info.width, raw_info.height, skip_left, skip_right, skip_top, skip_bottom);
                  

                  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.

                  1. Daniel Fort author

                    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.

                    1. Alex

                      Something like this should work:

                              if (!is_basic)
                              {
                                  /* update skip offsets */
                                  int skip_left, skip_right, skip_top, skip_bottom;
                                  calc_skip_offsets(&skip_left, &skip_right, &skip_top, &skip_bottom);
                                  raw_set_geometry(raw_info.width, raw_info.height, skip_left, skip_right, skip_top, skip_bottom);
                              }
                      

                      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.

                      1. Daniel Fort author

                        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 don't see any issues with black level:

                            Res:  1736x688
                            raw_info:
                              api_version      0x00000001
                              height           726
                              width            1808
                              pitch            3164
                              frame_size       0x00230CE8
                              bits_per_pixel   14
                              black_level      2047
                              white_level      16200
                              active_area.y1   28
                              active_area.x1   72
                              active_area.y2   722
                              active_area.x2   1808
                              exposure_bias    0, 0
                        

                        I'll shoot you a silent DNG tomorrow.

                        1. Alex

                          If the scene is mostly dark and the computed black level is close to 2047, it gets clamped to 2047. Maybe it was that.

  2. danne

    I´m on a tight schedule atm but will share my findings in general. Tested a july ML build yesterday, filmed 10bit. Have been filming crop_rec on my eosm this past summer and didn´t notice max resolution shrunk from 1728x692 to 1648x(? forgot exact height here). Check these files from the july build: https://bitbucket.org/Dannephoto/magic-lantern/downloads/M11-2146_crop_rec_samples.zip https://bitbucket.org/Dannephoto/magic-lantern/downloads/M11-2136_EOSM_3xzoom_samples.zip There´s no black bar on top. On recent 100D footage it seems width has to go all the way down to 660 to get rid of the top black but it probably could work all the way up to 692. I try to follow places in code but still can´t grasp how to fine tune or what needs to be done...

  3. Daniel Fort author

    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.

  4. Alex

    Alright, let's wait for some samples now (as I have yet to see a valid crop_rec/lossless DNG from 650D).

    1. Daniel Fort author

      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?

  5. Daniel Fort author

    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)"

    1. Alex

      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.

      1. Daniel Fort author

        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:

        if (is_EOSM || is_700D || is_100D)
        

        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.

    1. Alex

      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)

  6. danne

    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
    

    to:

            // 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
    

    And I get perfectly looking raw_diag image in crop_rec and resolution up to even 1728x698 with good looking images. https://bitbucket.org/Dannephoto/magic-lantern/downloads/1736x698_100D.MLV

    https://bitbucket.org/Dannephoto/magic-lantern/downloads/1736x698_100D_1_2017-10-14_0001_C0000_000001.dng

    "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?)

    1. Alex

      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).

      Math (ipython):

      In [1]: 1736*14//8*688 % 16
      Out[1]: 0
      
      In [2]: 1728*14//8*698 % 16
      Out[2]: 0
      
      In [3]: 1736*14//8*698 % 16
      Out[3]: 12
      

      More info: edmac-memcpy.c - edmac_copy_rectangle_cbr_start

  7. danne

    Adding crop_rec to this family in raw.c?

        /* params useful for hardcoding buffer sizes, according to video mode */
        int mv = is_movie_mode();
        int mv640 = mv && video_mode_resolution == 2;
        int mv720 = mv && video_mode_resolution == 1;
        int mv1080 = mv && video_mode_resolution == 0;
        int mv1080crop = mv && video_mode_resolution == 0 && video_mode_crop;
        int mv640crop = mv && video_mode_resolution == 2 && video_mode_crop;
        int zoom = lv_dispsize > 1;
    

    Thanks for edmac explanation.

  8. Daniel Fort author

    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
    

    So it makes sense why we would want to do this:

            #if defined(defined(CONFIG_EOSM) || defined(CONFIG_700D) || defined(CONFIG_650D) ||defined(CONFIG_100D)
            skip_top    = 28;
            skip_left   = 72;
            skip_right  = 0;
            skip_bottom = zoom ? 0 : mv1080crop ? 0 : 4;
            #endif
    

    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?

  9. Daniel Fort author

    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.

    1. Alex

      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...

  10. Alex

    Will merge it in this state, but will not enable a build yet - the lossless compression needs more investigation (not straightforward).