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 7a3b5fa3f4c6hg pull -r ml_dng https://bitbucket.org/dmilligan/magic-lantern# Note: This will create a new head!hg merge 114010a4155bhg commit -m 'Merged in dmilligan/magic-lantern/ml_dng (pull request #603)'
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).
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.
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.
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.
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.
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
Actually, it turned out this message does say it all. Before the cr2hdr-20bit merge the output looked like this:
$ make cr2hdr.exe
[ README ] module_strings.h
CROSS=1 make cr2hdr.exe
[ i686-w64-mingw32-gcc ] cr2hdr.exe
See my comment below for more.
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.
That's the clue I needed. Fixed it.
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.
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.]
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.
@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:
Shutter time, aperture and focal length should be correct or this is a FRSP bug? They aren't now on my EOS-M.
They are not compressed.
Is metadata correct in MLV? Is it correct for normal silent pics?
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:
Using DNGs, your patched ML has:
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).
Hint: 31.1 * 16 / 14 = 35.5
I've rewritten that part of my comment, although the issue stands.
Found the problem, looks like a result of writing to a pointer on non word boundaries. This works on x86 (where I developed this code for MLVFS), but not on ARM (http://stackoverflow.com/a/1238014/304326).
EDIT: problem fixed
Darktable opens ok
Also, UFRaw now complains twice (I get 2 dialogs) that:
a) which models was this tested on?
b) can you fix the conflicts?
c) which functionality is still missing/error prone?
Tested on EOS-M.
Some metadata is still missing, see my comments above.
Umm, what now?!:
Will NOT load on:
EOSM (round, floor)
Wondering if this was erroneous in the reports above too ^^^^^^^^^^^^^
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.
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).
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?
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.
[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.
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:
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.
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.
bouncyball patches? did i miss some pull request? if not, please submit a pull request, so i can add this.
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.
@bouncyball also did some other work that somehow isn't showing the mlv_dump dual iso artifact bug. The changes he made to dng.c broke cr2hdr so there's no pull request for that.
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 ;)
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.
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.
I forked the @dmilligan repository, made new branches and submitted the changes for each issue. I think that's the right procedure. Still learning so I had to revert a few commits to create clean pull requests.
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.
I can submit pull requests for the unified branch on the main repository but I had my reasons to do it this way.
The raw2dng white balance issue only affects the ml_dng version so patching the current unified has no effect.
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.
in issue #2612 you report that mlv_dump has a WB issue, caused by pull request #732 that was merged into unified.
this is said to be fixed on some other repository in some branch.
You pointed out cae0f12be3ca3d4429f46f6a48fd5b04861fb03d and it turns out that was causing the wb issue. This ml_dng branch didn't have that commit until the latest changes were merged into the ml_dng branch then the wb issue showed up on this pull request.
@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.
I'm getting a compile error on dng.c
That's not a compiler error. What is the full error? Did you compile ML core?
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...
[ 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
[ 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: *** [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.
I'm with you on this one as well @dfort!
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.
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.
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.
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.
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.