#603 Open
Repository
dmilligan
Branch
ml_dng
Repository
hudson
Branch
unified

Bitbucket cannot automatically merge this request.

The commits that make up this pull request have been removed.

Bitbucket cannot automatically merge this request due to conflicts.

Review the conflicts on the Overview tab. You can then either decline the request or merge it manually on your local system using the following commands:

hg update 7a3b5fa3f4c6
hg pull -r ml_dng https://bitbucket.org/dmilligan/magic-lantern
# Note: This will create a new head!
hg merge 114010a4155b
hg commit -m 'Merged in dmilligan/magic-lantern/ml_dng (pull request #603)'
Author
  1. David Milligan
Reviewers
Description

I thought I'd try and get all the DNG stuff I implemented for MLVFS back into the ML source.

Still TODO:

  • (DONE) Completely replace chdk_dng?
  • (DONE) Modify ML post processing apps (mlv_dump, cr2hdr, etc.) to use this code
  • (DONE) The white balance stuff isn't working yet (I've got to get that modified ufraw/dcraw code to compile).
  • Verify all metadata is correct

Comments (79)

  1. Alex

    Sounds interesting. I'll take a closer look, but it may take a while ( dmilligan knows why :P )

  2. Georg Hofstetter

    good idea. it is meant to encapsule the DNG writing code into a separate module by still being able to directly link it into desktop binaries?

  3. Daniel Fort

    In order to compile a Windows binary (Linux/MinGw) it needs -std=gnu99 in modules/lv_rec/Makefile Line 21. (At least that's what it needed on my system.)

  4. David Milligan author

    The PR is against the main branch, 'unified'. cr2hdr20bit has not been merged into unified yet, it's in its own branch, therefore, it's not really the 'official' version. When cr2hdr20bit is merged, then I will update this branch. If I merge it now, all of that stuff will be included in this PR, which is not the intent. They are separate. You can merge the branches yourself locally if you like. I have no idea if they will merge cleanly (probably not).

    1. Daniel Fort

      Fantastic--I was trying to do that by hand but gave up. Mac and Windows compiled cleanly but Windows gave an error that the file was too big to fit in memory. I'm using an old laptop with Vista so the file might be ok. The Mac version checked out fine. Let's get these to people willing to run some tests and see how it goes.

      Windows cr2hdr20bit

      Macintosh cr2hdr20bit

  5. Daniel Fort

    Should we continue to discuss cr2dng20bit here or comment on the page for that branch?

    The Mac to Windows cross compile system that worked before isn't working for this version. There are no compiler warnings but the binary doesn't launch in Windows. The Mac binary seems to be fine. Note that I also tried compiling in Cygwin and MinGw on a Windows system and the Cygwin version compiled and launched successfully.

    Maybe a problem is that this branch is so far behind unified? I'm thinking that because I got a "truncate: illegal option -- s" in src/Makefile.src that was fixed a long time ago. It is a problem with the MacPorts version of truncate.

    1. David Milligan author

      truncate doesn't have anything to do with cr2hdr (at least, I don't see it being used in any cr2hdr makefile rules). I don't think there's anything wrong with the code or the makefiles. It sounds like just a compiler/configuration issue on your end.

      1. Daniel Fort

        I should say that the truncate issue came up when I tried to build ML for EOSM. Nothing really to do with cr2hdr but thought I'd give it a try because when building ML for a camera platform it also builds cr2hdr. I thought maybe some environmental variables were setup a little different doing it this way? Guess not.

      2. Daniel Fort

        Kind of hard to figure out what is wrong with my compiler/configuration when the output looks like this:

        $ make cr2hdr.exe
        CROSS=1 make cr2hdr.exe
        [ gcc      ]   cr2hdr.exe
        

        [EDIT]

        Actually, it turned out this message does say it all. Before the cr2hdr-20bit merge the output looked like this:

        $ make cr2hdr.exe
        Updated HGVERSION
        [ README   ]   module_strings.h
        CROSS=1 make cr2hdr.exe
        [ i686-w64-mingw32-gcc ]   cr2hdr.exe
        

        See my comment below for more.

  6. Daniel Fort

    I just found out what the problem is with the cr2hdr 20-bit Windows cross compile, though I don't have a suggested solution for it yet.

    The cr2hdr.exe created isn't a Windows binary, it is a binary of the host system. At least that's what happened on my Mac. All other command line tools I've been cross compiling (raw2dng and mlv_dump) are cross compiling properly, that is they won't launch on a Mac but do launch on Windows. That's as far as I've gotten in my testing.

    I checked the other branches, ml_dng, cr2hdr-20bit and unified and the only branch that has this cross compiling issue is ml_dng-cr2hdr20bit.

  7. Daniel Fort

    That worked perfectly--you're awesome!

    Not to let anything fall through the cracks--there's also a cross compile issue with raw2dng:

    $ make raw2dng.exe
    [ MINGW    ]   raw2dng.exe
    ../dng/dng.c: In function 'dng_write_header_data':
    ../dng/dng.c:539:5: error: 'for' loop initial declarations are only allowed in C99 or C11 mode
         for(int i = 0; i < COUNT(cam_matrices); i++)
         ^
    ../dng/dng.c:539:5: note: use option -std=c99, -std=gnu99, -std=c11 or -std=gnu11 to compile your code
    make: *** [raw2dng.exe] Error 1
    

    Adding -std=gnu99 to line 21 of Makefile takes care of that.

  8. Daniel Fort

    Now that raw2dng, cr2hdr and mlv_dump are compiling and launching on Mac and Windows, time for bug reporting.

    ./mlv_dump -o test1 --dng --cs2x2 -v M20-1742.MLV
    

    results in a "Segmentation fault: 11" and no .dng files created. Current unified version works as expected.

    [EDIT: Issue only affects Mac version. Tested on Windows and it seemed to work fine. Looks like bugs should be reproduced on all platforms--Linux too?]

    [EDIT2: Disregard this bug report. Turns out that bug is in the ml_dng-cr2hdr20bit branch but not in ml_dng branch. Sorry for the confusion.]

  9. Antonio Rodrigues

    Just tested this, and it corrects the double tags problems, as mentioned in the ML forum POST. This problem hapens in Rawtherapee versions 4.2.188 to 4.2.276. Using a canon 550D.

    Now no need to run the exiftool to correct this.

  10. Daniel Fort

    @dmilligan don't know if you want to deal with it on this pull request but the Makefiles for cr2hdr and raw2dng look for i686-w64-mingw32-gcc in the user's home directory while mlv_dump looks for i686-w64-mingw32-gcc in the path environmental variable. I have suggested adding -std=gnu99 when compiling with the i686-w64-mingw32-gcc but it looks like that is already done for you if you use what is in Makefile.user.default:

    #
    # mingw cross compiler settings
    # 
    MINGW=i686-w64-mingw32
    MINGW_GCC=$(MINGW)-gcc
    MINGW_AR=$(MINGW)-ar
    
    MINGW_CFLAGS=-g -O3 -W -Wall -fms-extensions -std=c99 -m32
    MINGW_LFLAGS=
    MINGW_LIBS=
    MINGW_LFS_FLAGS=-DLARGEFILES -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 
    

    Just a suggestion that would make setting up a development system a bit less kludgy.

    [EDIT - I made a pull request that addresses this but you could do a much better job on cr2hdr.]

    [EDIT2 - Should have looked at your ml_dng-cr2hdr20bit branch -- already did this back on Aug 20, you're way ahead of me.]

    https://bitbucket.org/hudson/magic-lantern/pull-requests/659/eliminate-need-for-mingw-w32-bin-i686-w64/diff

  11. nikfreak

    @dmilligan: any chance this might end up as a "chdk-dng-replacement" module so that it reduces autoexec.bin size soon?

    1. David Milligan author

      It already does replace chdk_dng. The only core code that did DNG writing was debug code anyway, mostly it's just used by silent.mo.

  12. David Milligan author

    They are not compressed. Is metadata correct in MLV? Is it correct for normal silent pics?

    1. Licaon Kter

      MLV metadata is correct.

      Normal silent pics still wrong.

      Also, ok they don't appear compressed, using official ML from Dec 21 yields similar sized images, albeit those CAN be opened by Darktable so there's another issue actually.

      Using DNGs, normal ML has:

      • correct model
      • correct maker

      Using DNGs, your patched ML has:

      • correct model
      • correct maker
      • correct lens
      • correct ISO
      • correct date/time
      1. David Milligan author

        Look at dng.c below at line 557. They are not compressed in any way whatsoever, and you'll notice the complete lack of any sort of compression code. The size of the DNGs is exactly W x H x 14 / 8 bytes + DNG header size (several KB).

        1. Licaon Kter

          I've rewritten that part of my comment, although the issue stands.

          How do I compare 2 DNGs ( or headers maybe) ?

              1. David Milligan author

                Could you upload the PR DNG itself? "Warning = bad IFD1 directory" is concerning. That's the ifd with exif, so could be the reason for both issues (missing metadata, won't open in darktable).

                    1. Licaon Kter

                      Correct:

                      • Darktable opens ok

                      Still wrong:

                      • shutter time
                      • focus distance

                      Also, UFRaw now complains twice (I get 2 dialogs) that:

                      Image: Lensfun: Vignetting: Model: k1: Value -1,777 too small, truncated to -1,000.
                      
  13. Georg Hofstetter

    a) which models was this tested on? b) can you fix the conflicts? c) which functionality is still missing/error prone?

    br, g3gg0

  14. Licaon Kter

    Umm, what now?!:

    Will NOT load on: 
        EOSM (round, floor)
    

    Wondering if this was erroneous in the reports above too ^^^^^^^^^^^^^

  15. Daniel Fort

    What is the state of this pull request? Reason I ask is because @bouncyball has reworked mlv_dump in your ml_dng branch and fixed a long standing white balance issue. He gave me permission to put in a pull request for it, provided I can resolve the conflicts and don't loose any of the new mlv_dump features of course. However, I'd like to get your ok before proceeding because this would involve including the dng module in unified, replacing makefiles, reworking raw2dng and perhaps other changes. In other words much of what this pull request would accomplish.

    This is my progress so far: https://bitbucket.org/daniel_fort/magic-lantern/branches/compare/daniel_fort/magic-lantern:ml_dng-mlv_dump%0Dhudson/magic-lantern:unified

    [EDIT] I got it working and am ready to post a pull request.

      1. Daniel Fort

        Hum, the 70D cam_matrices are missing from modules/dng/dng.c. I'll ask @andy600 for help on this. Would that be all it takes to fix it for the 70D?

        Edit: Never mind, found it in MLVFS.

    1. David Milligan author

      Re-merged with the latest changes. Not tested but it compiles. I don't see how raw2dng could have "serious white balance issues" given that the raw format doesn't record any white balance information and therefore it's impossible for the white balance to be anything but wrong. If you don't like what the white balance is hard coded to, then just change it (raw2dng.c line 59-65).

        1. David Milligan author

          I don't see how that's an issue or why it matters. The white balance is always wrong. There's no way white balance can be correct.

    1. David Milligan author

      yes, the latest merge from unified just needs some minimal testing (the majority of the code has been in use in MLVFS and elsewhere and is pretty well tested and mature), here's a summary of changes:

      • move dng writing code to a module
      • replace chdk-dng.c with dng.c from MLVFS
      • allow 64bit compilation of post processing apps
      • use HOST_CC variable instead of hard-coded "gcc" in makefile for raw2dng
    2. Daniel Fort

      There is still an issue with raw2dng but everything else I tested (mlv_dump, cr2hdr) is working fine including a problem with the current mlv_dump that you know about. https://bitbucket.org/hudson/magic-lantern/issues/2616/artifacts-showing-up-on-dual_iso-with

      The strange thing about this bug is that it doesn't show up in the @bouncyball ml_dng branch when I merged in the latest changes from unified. Maybe something with his dng.c changes? There are no differences between mlv_dump.c in this pull request and the one that I'm working with in my test branch.

      I haven't tested silent pictures yet, are there any issues there?

  16. danne

    Been running the ml_dng code for a long time in MLP. In mlv_dump that is. Rock solid with correct wb tag amongst other great cdng tags.

    1. Daniel Fort

      Slightly off?

      [EDIT] Danne edited his comment so my "Slightly off?" comment took on a different meaning.

      I just shot another test and this is what is happening. First of all the current unified version. current_raw2dng.jpg

      I get it that it defaults to 5500K. In ACR it shows a temp of 5050 and a tint of +8

      Using raw2dng from this ml_dng pull request I'm seeing this:

      ml_dng_raw2dng.jpg

      That's not slightly different. ACR reports a temp of 7000 and a tint of +150.

      Note that @bouncyball added an option to use the metadata from a MLV file so I shot an mlv at the same time and processed it using the sidecar option and it looks like this.

      sidecar_test.jpg

      In this case ACR is showing a temp of 5200 and tint of +11. That's a subtle change from the default. Note that I'm using daylight so I suppose that something shot in tungsten or fluorescent lighting will look "wrong" unless you use an mlv file with the sidecar option to correct the color.

      So everything is working fine except the default white balance. I looked at raw2dng.c line 59-65 but I haven't been able to "fix" it yet.

      @dmilligan - I feel your pain. If we can replace raw 1.0 with your 1.1 version we wouldn't need to deal with raw2dng.

      1. Georg Hofstetter

        bouncyball patches? did i miss some pull request? if not, please submit a pull request, so i can add this.

          1. Georg Hofstetter

            well, don't get me wrong. but that doesn't help ML development at all.

            i just see some fork-off with a patch that says "mlv_dump dng output fixes" for example and a few "global" changes too. and when merging the branch into the main, i am quite sure it contains 64 different stray changes that will not merge nicely.

            please just create a fork, incorporate a small change like "fix mlv_dump crash situation" and file a pull request.

            i will then accept that pull request and the bug is fixed for everyone, not just the two people that use bouncyball's repository.

          1. Georg Hofstetter

            i guess he didnt sync to unified for a long time. thus his version doesnt contain code that broke vertical stripe fix. but just a wild guess ;)

            1. Daniel Fort

              Actually I was trying to combine the dmilligan and bouncyball ml_dng branches and the vertical stripe fix didn't cause that issue we're seeing with dual iso in my test branch. I ran into the same problems I'm reporting here with raw2dng and a different issue with cr2hdr. I'm turning my attention to testing this pull request now that all the latest changes have been merged into it.

      2. David Milligan author

        Simply change those values from 0 to something valid (e.g. wb_mode = WB_DAYLIGHT)

        The wb calculations use some code from ufraw. It results in slightly different temperatures than those computed by ACR. It's not perfect, but it's probably the best we can hope to do without expending tremendous effort reversing ACR's color models. These computations use very complex matrix math and color science that is just a little over my head.

        1. Daniel Fort

          Well that didn't work:

          raw2dng.c:59:16: error: use of undeclared identifier 'WB_DAYLIGHT'
              .wb_mode = WB_DAYLIGHT,
                         ^
          
  17. DeafEyeJedi

    Excellent progress. Funny MLP has had this fix for long that I somehow had no idea it was broken elsewhere. Very nice!

  18. Daniel Fort

    Submitted a couple of fixes as pull requests:

    • White balance hardcoded to daylight in raw2dng - it was hardcoded to unknown
    • Backed out commit in raw2dng that was causing problems with vertical stripe fix on dual iso (might be getting fixed before this gets merged so it can serve as a possible conflict fix)
        1. Georg Hofstetter

          yeah but fixes for some mlv_dump thing, somewhere buried in a "lets add a DNG module"-pull-request of a fork of the original is just a bad idea.

          monster-pull-requests are hard to merge, touch a million of files for totally diferrent reasons. please, if you've got a fix for something, fork the original, file a pull-req and it will get merged immediately while this PR will (and already does) take ages until it is merged.

          if alex now patches the artifact issue in unified (or any other thing), this pull request isnt anymore able to merge.

          1. Daniel Fort

            I can submit pull requests for the unified branch on the main repository but I had my reasons to do it this way.

            1. The raw2dng white balance issue only affects the ml_dng version so patching the current unified has no effect.

            2. You already pointed out the dual iso artifact issue to a1ex so I felt that it was inappropriate for me to step in and submit a pull request for that. In addition I don't understand the vertical stripe fix code so I don't feel qualified to defend the pull request. Submitting the pull request here is to help with a clean merge should it be fixed in the unified branch before this ml_dng pull request is accepted (a likely scenario) though I'm sure dmilligan needs no help from me resolving merging conflicts.

            If this is wrong I can decline my pull requests and submit them to the main unified branch. Just let me know.

  19. Daniel Fort

    @dmilligan Thanks for merging the cam_matrices for the 100D -- cam_matrices for the 500D and camera_focal_resolution for the 100D are still missing. (Note that this is also missing in MLVFS.)

    [EDIT] Danne showed me how to find this information. Submitted pull request to update dng.c.

    1. David Milligan author

      That's not a compiler error. What is the full error? Did you compile ML core?

      1. Daniel Fort

        Did I compile ML core? Not sure what you mean. I usually just cd into the individual camera directory and "make zip" to build. Always worked for me. I also tried "make EOSM" from the main directory which seemed to compile the ML core but then going to the platform/EOSM.202 directory and doing a "make zip" still doesn't build the dng module. Here's the full error:

        Building module dng...
        Updated HGVERSION
        [ README   ]   module_strings.h
        [ CC       ]   dng.o
        dng.c: In function 'temperature_to_RGB':
        dng.c:315:24: warning: implicit conversion from 'float' to 'double' to match other operand of binary expression [-Wdouble-promotion]
                 xD = 0.27475e9 / (T3) - 0.98598e6 / (T2) + 1.17444e3 / T + 0.145986;
                                ^
        dng.c:315:43: warning: implicit conversion from 'float' to 'double' to match other operand of binary expression [-Wdouble-promotion]
                 xD = 0.27475e9 / (T3) - 0.98598e6 / (T2) + 1.17444e3 / T + 0.145986;
                                                   ^
        dng.c:315:62: warning: implicit conversion from 'float' to 'double' to match other operand of binary expression [-Wdouble-promotion]
                 xD = 0.27475e9 / (T3) - 0.98598e6 / (T2) + 1.17444e3 / T + 0.145986;
                                                                      ^
        dng.c:319:24: warning: implicit conversion from 'float' to 'double' to match other operand of binary expression [-Wdouble-promotion]
                 xD = -4.6070e9 / (T3) + 2.9678e6 / (T2) + 0.09911e3 / T + 0.244063;
                                ^
        dng.c:319:42: warning: implicit conversion from 'float' to 'double' to match other operand of binary expression [-Wdouble-promotion]
                 xD = -4.6070e9 / (T3) + 2.9678e6 / (T2) + 0.09911e3 / T + 0.244063;
                                                  ^
        dng.c:319:61: warning: implicit conversion from 'float' to 'double' to match other operand of binary expression [-Wdouble-promotion]
                 xD = -4.6070e9 / (T3) + 2.9678e6 / (T2) + 0.09911e3 / T + 0.244063;
                                                                     ^
        dng.c:323:24: warning: implicit conversion from 'float' to 'double' to match other operand of binary expression [-Wdouble-promotion]
                 xD = -2.0064e9 / (T3) + 1.9018e6 / (T2) + 0.24748e3 / T + 0.237040;
                                ^
        dng.c:323:42: warning: implicit conversion from 'float' to 'double' to match other operand of binary expression [-Wdouble-promotion]
                 xD = -2.0064e9 / (T3) + 1.9018e6 / (T2) + 0.24748e3 / T + 0.237040;
                                                  ^
        dng.c:323:61: warning: implicit conversion from 'float' to 'double' to match other operand of binary expression [-Wdouble-promotion]
                 xD = -2.0064e9 / (T3) + 1.9018e6 / (T2) + 0.24748e3 / T + 0.237040;
                                                                     ^
        dng.c:325:30: warning: implicit conversion from 'float' to 'double' to match other operand of binary expression [-Wdouble-promotion]
             yD = -3 * xD * xD + 2.87 * xD - 0.275;
                                      ^
        dng.c: In function 'kelvin_green_to_multipliers':
        dng.c:434:42: warning: implicit conversion from 'float' to 'double' to match other operand of binary expression [-Wdouble-promotion]
                     chanMulInv += 1 / pre_mul[c] * cam_rgb[c][cc] * rgbWB[cc];
                                                  ^
        [ HGDIFF   ]   hgdiff.tmp
        [ MODULE   ]   dng.mo
        [ STRIP    ]   dng.mo
        [ OBJCOPY  ]   dng.mo
        [ EXPORTS  ]   dng.sym
        000001f0 dng_write_header_data
        00001bb0 reverse_bytes_order
        00001c78 dng_free
        00001ca8 dng_get_info
        00001e58 dng_save
        [ DEPENDS  ]   dng.dep
        Will NOT load on: 
            EOSM (round, floor)
        Not checked (compile ML for these cameras first):
            1100D, 500D, 50D, 550D, 5D2, 5D3, 600D, 60D, 650D, 6D, 700D, 7D
        make[4]: *** [dng.dep] Error 1
        

        Just an aside, a few of us have been using mlv_dump from your ml_dng branch because it is an improvement over the unified version. It doesn't have some of the bugs and it creates cdng files that will open in Adobe Premiere. I'm trying to provide fixes for the few problems that we have encountered with this branch. What will it take to get this pull request merged? No rush, we're all busy but this pull request has been open for nearly a year.

    2. Daniel Fort

      I did some regression tests and it looks like it was at commit e8b5c7c that the issue first appeared. Before this commit dng.dep was being created and the module would build successfully.

      1. David Milligan author

        dng.dep is simply a dependency check, there's a python script that makes sure the ML core actually contains the functions that the module tries to use. That's why I asked if you had compiled the ML core (if you don't, the dependency check will always fail). Notice how it says Will NOT load on: EOSM (round, floor). It's saying that the module compiles fine but it's trying to use two functions, round and floor, that couldn't be found in the EOSM's autoexec.bin (ML core). These functions are actually standard libc functions, The reason they are not there is because the ML build system tries to save space by not including libc functions that are not used. Unfortunately it only checks the ML core for whether or not those functions are used. By the time you compile a module, those functions have been stripped away. The solution is that in one of the makefiles you can explicitly force the build system to include those functions and not strip them away.

        1. Daniel Fort

          I understand what you are saying, well sort of--I'm still learning. I can't see anything in the commit I pointed out that would cause the issue because none of the makefiles were changed. I'm using SourceTree so I'm able to regress to a specific commit and what I'm seeing is:

          commit e8b5c7c (14282 in SourceTree) and later -- dng module doesn't build because the dependency check fails -- dng.dep file not created.

          commit 3c016ad (14281 in SourceTree) and earlier -- dng module builds -- dng.dep file is created.

          Assuming you're doing a 'make clean' from the ML root you aren't seeing this? From a clean start I'm going into the camera platform directory and doing a 'make zip', that has always worked for me before. I don't understand what changed between these two versions that would cause this to happen.

          1. David Milligan author

            That broken changeset add some code that used floor() and round(). That is why it fails. It was later corrected like I said. See the diff of src/Makefile.src.config below. The current branch compiles fine.

            1. Daniel Fort

              Found the problem, my bad. I was working with the ml_dng branch in the hudson repository. I should know better since I made pull requests to your repository. I see that my "Oops--dumb mistake." commit got into your branch.

              Note that there are two more PR. One that fixes the raw2dng white balance issue we discussed a while ago and another for the missing 500D and 100D data: https://bitbucket.org/dmilligan/magic-lantern/pull-requests/

              Once again sorry for my beginner's mistakes.