CMOS/ADTG/Digic register investigation on ISO

Started by a1ex, January 10, 2014, 12:11:01 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Danne


Danne

I tested iso 200 with the new tweaks and then with a build at iso 200 without the tweaks. Note. Only very briefly and the test scene wasn´t even static but settings were exactly the same:

Iso 200 no modification(comes out brighter)


Iso 200 modified(comes out darker)


iso 200 unmodified


iso 200 posterization in modified(more testing needs to be performed)?


Ok, so I have no interest in proofing anything bad here. On the contrary I really want this to work. I would suggest someone to do following test.
1 - Record at iso 200 with the exact same settings with modified and unmodified version. Even if brightness will vary.
2 - Match brightness in post and compare shadows for refinements or not.
3 - Maybe try other isos as well and do above test

Recording scene should be chosen with shadows in mind.
If we could take the next step by doing these tests we will immidieately understand if the registry changed are the correct ones etc.

timbytheriver

Let's remind ourselves of @a1ex's recent apropos post here first! https://www.magiclantern.fm/forum/index.php?topic=24601.msg221994#msg221994  :)

Here are my comparisons. Black Strength lowered to 0 to bring up shadows in all. Strongly suggest looking at full-res (Load Full Resolution) on the upload site.

1) Stock ISO 200


2) Mod ISO 200 – Brightness matched in MLVApp


3) Mod ISO 200 – Exp raised in camera to compensate for reduced gain ceiling (spotmeter now maxes-out at 95% on the hole of the lampshade)


I know which combination of settings I'd prefer grading from. :)

PS @Danne I'm not seeing posterization [yet]!
5D3 1.1.3
5D2 2.1.2

Danne

Not at my computer atm but how are the first two files when compared? Identical? Will take a look in a while.

If you increase brightness like in third example we again are not comparing in a good way imo.

timbytheriver

I'd say that the mod changes the quality of the noise for the better in 2) (matched brightness)

If the exposure were simply brightened without the mod, the highlights would blow. So with the mod + recorrected exp ceiling we retain the complete dynamic range of stock ISO  – but with marked improvement in shadow noise.

Maybe we should be thinking of this as a way of exposing rather than just settings. ?
5D3 1.1.3
5D2 2.1.2

Danne

That's the other aspect. Modifying exposure behaviour as a user. Wouldn't it only act as in cam programmed ettr compensation? I mean. It's of course positive to be able and expose "normally" but let's keep the information real.

timbytheriver

I hear you, but I'm not necessarily suggesting modifying ML ISO behaviour across the board at a deep system level. I'd be happy to see this as a "let's turn on the 'clean-iso preset' and ETTR to the new exposure ceiling because this shot would really benefit from cleaner shadows..."

That's why I'd prefer to see it working as Q-sub-menu choice as opposed to a hard-coded preset like I have at present. Then at least those who wanted to use it could do so, and others could happily ignore it. :)
5D3 1.1.3
5D2 2.1.2

Danne

Sure. Just go ahead. You came much longer than I ever did in this subject.
Looking forward to the continuation.

timbytheriver

5D3 1.1.3
5D2 2.1.2

70MM13

here's some graphs for "tnt" iso 120 (1/50s) vs canon iso 200(1/60s)...

i'm not certain that the SNR graph for 120 is correct because it was generated immediately after changing modes, so the amplifiers were possibly "unsettled".

a delay of 30 seconds may be needed in the diag module to ensure correct readings!

the black level dynamic range measurement is definitely correct, however.  i could see on the camera display that there were no issues with stripes or green colours during that phase of the task.

any insights would be appreciated!

ob-dr-120" border="0 ob-dr-200" border="0 snr-120" border="0 snr-200" border="0

never mind, the two step graph worked...




timbytheriver

Hi

In several different 5D3 builds incl: my own, danne's, a1ex's (with no reg changes!) I'm having big problems with taking the second picture of the SNR 2 shots in raw_diag module. 9/10 I am getting 'BUSY' displayed, and the camera locks/needs batt pull. I get no second picture saved.

I notice also that when the camera is sitting there OFF, the red light pulses every few seconds... This doesn't happen consistently though. :( Something odd going on...


Could someone offer some help please?
5D3 1.1.3
5D2 2.1.2

timbytheriver

I have uninstalled ML completely. Restored Canon FW. Reinstalled ML.

Things are working again in basic crop_rec mode, however I can't get raw_diag.mo to show the small liveview reporting window (EV etc).

Is there a definitive version of raw_diag.mo I should be using?

Also, there is a message in Debug > (Red dot) Memory Patches - 1 ROM,2 RAM and a message in yellow text at the bottom: "Cache is locked down (not exactly clean).

Is this telling me something's wrong?


5D3 1.1.3
5D2 2.1.2

a1ex

Well... did anyone say these things are ready for the grand public? :D

It's certainly possible that some things were broken during "recent" commits; you may very well be the first person trying these things in several years. If in doubt, you could go back in history (older commits on the iso-research branch) and see if anything worked better in the past, then use "hg bisect" to identify where things went wrong. Caveat: there was no crop_rec back then.

Confirmed this particular bug on 5D3 1.1.3 with latest iso-research build. In LiveView it appears to work, as long as Image Review is "finite" in Canon menu (e.g. 2 seconds, rather than Hold).

timbytheriver

@a1ex Thanks!
Quoteready for the grand public? :D
I consider myself part of the great unwashed! ;)

Nice catch. The diag box does now appear! :) But to operate the 2-shot it has to be turned back to 'Hold' – whereupon my camera still goes 'Busy' and locks up. Will research "hg bisect".

Cheers!
5D3 1.1.3
5D2 2.1.2

a1ex

It seems to be some memory management issue; that's the usual cause for the BUSY message. For some reason, there is memory allocated from the SRM buffer, that has to be freed before the camera is able to take another picture (that's a limitation from Canon firmware).

Here's what I did to narrow it down:

1) some changes in mem.c to log the activity of "large" memory allocators (shoot_malloc and srm_malloc, or anything else other than the general-purpose ones):


diff -r 75c1f3fe2927 src/mem.c
--- a/src/mem.c
+++ b/src/mem.c
@@ -30,2 +30,3 @@
#endif
+#define exm_printf(fmt,...) { if (allocator_index > 2) printf(fmt, ## __VA_ARGS__); }

@@ -811,3 +812,4 @@

-        dbg_printf("using %s (%d blocks, %x free, %x max region)\n",
+        exm_printf("alloc(%s) from %s:%d task %s\n", format_memory_size_and_flags(size, flags), file, line, current_task->name);
+        exm_printf("using %s (%d blocks, %x free, %x max region)\n",
             allocators[allocator_index].name,
@@ -875,3 +877,3 @@

-    dbg_printf("free(%x %s) from task %s\n", buf, format_memory_size_and_flags(((struct memcheck_hdr *)ptr)->length, flags), current_task->name);
+    exm_printf("free(%x %s) from task %s\n", buf, format_memory_size_and_flags(((struct memcheck_hdr *)ptr)->length, flags), current_task->name);


You could also enable all the debug messages from there (define MEM_DEBUG in mem.c), but they are very verbose; the only way to see them is to save them to card (also enable CONSOLE_DEBUG in console.c).

2) this pointed to a 31MB block allocated from raw_diag.c at line 566 (where it allocates something named "second_buf" to copy one of the images there)

3) annotated raw_diag.c to see when this part of the code was changed, and why (hg blame modules/raw_diag/raw_diag.c | grep second_buf)

Result: this part of the code is the one from 2014. If anything, the memory backend is handling this large malloc request differently (in the past it used shoot_malloc, now it prefers SRM), so we need to either provide some hints to the memory allocator, or to tweak the heuristics in mem.c. This is not exactly well explained in comments, but the preference for shoot_malloc vs srm_malloc is controlled by MEM_TEMPORARY and MEM_SRM (which should be renamed to something more consistent, as the SRM buffer is something that has to be freed right away, but you can get away with using the "shoot" buffer for a while). The first "temporary" flag was created when we only knew about the existence of the "shoot" buffer (which shouldn't be kept allocated during the entire ML session, otherwise ERR70 with certain operations, so... that memory should be only "temporarily" allocated, i.e. freed when we are done with it), and then we discovered SRM buffers, which have even more usage restrictions (you can't keep any of these allocated if you want to take a picture with Canon routines, and you *have* to free them in exactly the order they were allocated).

So, using tmp_malloc in raw_diag.c for that large buffer, fixes the issue, but these wrappers should really be renamed to something that makes sense. Maybe @names_are_hard has a better idea here :)




Edit: new build on the experiments page (commit f7947b6).

Now, the question is whether to trust these numbers; the biggest issues are, in my opinion:
1) white level should be manually checked for correctness
2) sensor response curve should be evaluated (there are tweaks that affect the response curve, from what I could tell, and the current code assumes perfect linearity)

A lower-hanging fruit might be automating the whole procedure (e.g. with Lua scripts) and checking its repeatability against various test scenes, settings etc, but definitely time-consuming. In theory, this should give correct results on any out-of-focus scene covering all tone ranges from deep blacks to overexposure (detailed requirements here).

I'm tempted to build a dark box with software-controllable lighting, similar to the one at Apertus, but I doubt it will happen during this holiday.

timbytheriver

@a1ex Many thanks! Very thorough explanation. Have updated to latest raw_diag.mo

It's a whole lot better – but I'm still occasionally (2 out of 10?) getting the 'Busy' on second SNR shot. Sometimes I can settle the regs with a MENU button press. Other (now rare) times the camera locks (batt pull). But it seems certainly workable with now! :) Is this relevant: Debug > (Red dot) Memory Patches - 1 ROM,2 RAM and a message in yellow text at the bottom: "Cache is locked down (not exactly clean). A Ram management issue?

QuoteNow, the question is whether to trust these numbers;

Funny you should say this... ;)

I see a whole EV DR readout difference between the small readout window say at DR 11.62 and saved chart: EV 10.88

And the histogram often tells another story... Which to trust?  :-\

@anyone I think these have come to light as the testing of my Q-iso-regs build requires the clip point to be set exactly for the benefit to be seen at its best.




My SNR charts look a bit left biased. Shouldn't the curve be to the right more – given the scene DR?
5D3 1.1.3
5D2 2.1.2

a1ex

The small readout window uses a very crude approximation, but it's computed quickly: log2(white - black) - log2(noise_stdev); look at the comments in the source code. The other one measures from white level until the level where SNR = 1.0 = 0 EV (as defined in ISO 15739); details here. Both depend on whether the white level is guessed correctly, and on the linearity of the sensor response.

FYI:
- I cannot see your screenshots without looking at the source code of your post (JS issues? edit: nope, ibb.co is now blocked by uBlock Origin)
- you've got screenshots saved on the card, no need to take pictures of the camera screen ;)

timbytheriver

@a1ex All good stuff to know. Many thanks for sharing.  :)

QuoteI cannot see your screenshots without looking at the source code of your post (JS issues?)

How odd. I'm seeing them on my browsers (Chrome & Safari on Mac)! I used https://imgbb.com/, which usually works ok.  :-\ Anyone else not seeing my images preview?
5D3 1.1.3
5D2 2.1.2

timbytheriver

I'm trying to pass a default value to some registers for which I can't find any [default] values listed in adtg_gui or iso_regs.

Is there some code that says: 'set to default value' even if that is not known?

Something like this:

adtg_new[25] = (struct adtg_new) {6, 0x14, default_value};


Thanks!
5D3 1.1.3
5D2 2.1.2

names_are_hard

Can you give an example of what you mean?  I can't understand your question.  Can you show the existing code that does similar to what you want?  What do you expect the default value to hold in your new case?

timbytheriver

Hey!
Certain registers such as ADTG Preamp et al are set at Canon default values at boot.

So the above example (ADTG Preamp) loads default values of: 0x60,0x61,0x62,0x60 and 0x63,0x65,0x63,0x63 for the various columns/colour channels. These values are discoverable through the documented work of others in adtg_gui.c and iso_regs.c

There are other register for which I can't find values that load at boot i.e. 'default' values, that I wish to address. I was asking whether there is some code that says "load default value if discoverable, or set a safe value as default" – rather than have me set some crazy inappropriate value of my own!  :o

Hence my crude attempt:

adtg_new[25] = (struct adtg_new) {6, 0x14, default_value};

Does that make sense?  :)
5D3 1.1.3
5D2 2.1.2

names_are_hard

More sense, at least :)  I might make that code look like this:


set_adtg_defaults(adtg_new, adtg_new_size);  // does whatever the "discoverable" algorithm is for all registers
adtg_new[25] = (struct adtg_new) {6, 0x14, 0xdead};  // override the default with 0xdead


If you can set to a safe value, simply set it for all registers, then override after that.  You say it's possible to discover safe values, so make the set_adtg_defaults() function do that process for each register.  If it can't find one for a register, simply skip it.

I wonder, do you mean "initial", not "default"?

timbytheriver

Thanks! "initial" is certainly a better way of describing my "default" yes!  :)

Since I know certain initial values (which I already assign to certain regs) it looks like your code might override all adtg_new?  – whereas I want to set "initial" to only certain regs, on a line by line basis – and then override it.

Might that be something like?:

set_adtg_defaults(adtg_new[25], adtg_new_size);  // sets whatever the "discoverable" algorithm is for only adtg_new[25] register.
adtg_new[25] = (struct adtg_new) {6, 0x14, 0xdead};  // then override the initial of adtg_new[25] with 0xdead


PS Here's how I'm doing the existing overrides which is working:
https://bitbucket.org/rivertim/magic-lantern-danneclone/commits/7fce7f696aa1ae67c4aa16248c497a612bc10761
(Not sure why that file is marked 'DRAFT', as I committed and pushed changes to that repo days ago!?)
5D3 1.1.3
5D2 2.1.2

names_are_hard

Initial vs default changes it significantly.  Yes, I was intending to set all registers, that makes sense with a default.  There's still enough registers that I think a function to initialise them is reasonable, but now it would only set those where good values are known.  You'd still want to pass in adtg_new directly, so that you can access all members of the array.  The logic (and the name) of the function would change.  Note that I didn't provide any code for the body of the function!  I was only trying to provide a conceptual outline.  You don't have to do this, the existing code doesn't (would be cleaner in my opinion if it did).

Going back to your original question, "initial" makes me confused again:
Quote
Is there some code that says: 'set to (initial) value' even if that is not known?

How are you expecting that to work?  If there's no known value, what can the initial value be?  You have to assign a distinct value in C when you do assignments, C doesn't have the concept of abstract or ranged values, if that's what you mean (some languages do have this).

Looking at your code, I think you have an integer overflow bug when you're setting registers using 0x400 + reg_x pattern.  Because the addresses you're assigning to are later considered to be 2 bytes wide, and menu input + the constant could make this higher than uint16_t max.  This could lead to menu choices giving the user a radically different result than they expect.  You should probably clamp the value to between 0 and uint16_t max.  Also since your variable is signed, underflow will lead to sign extension and really mess your values up.

You should rename reg_8x to reg_8882 (or something reg_XXXX at least), the current name implies it's 1 byte wide which has real potential for confusion.

Then I looked at the code that does the register assigns and it's really wonky.  data_buf is uint32t*, but all assignments cast it to uint16_t*, and the values that are read from are plain int?  That's pretty confusing - it also feels like it should interact with memory alignment on ARM, but I don't know ARM well enough to be sure.

Audionut

It sounds like you are saying all of the register values (in adtg) have been found and hard coded (so that we know what they are and can call upon them).  That doesn't sound like something a1ex would do.  Most of the values are different for each camera and at different shooting modes for each camera.

In adtg after adjusting a value you can press set (or some other button I don't recall) to return the register to initial value (before modification).  Does that code help?