Assign lens focal length and name for non cpu lenses

Started by Lars Steenhoff, October 29, 2016, 12:04:45 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

dfort

Noticed that simple silent pictures (MLV option) is broken with my manual_lens_info -> crop_rec_4k_mlv_snd merge attempt.

This is with only the silent module with either mlv_lite or mlv_rec active -- the lua module is not active in this case just to isolate the problem:



FRSP seems to be fine.

Resolving this issue will probably get us a step closer to getting ELNS (extended lens metadata?) working on the crop_rec_4k branch.

Lars Steenhoff

I just tested the branch from dfort and the results are good for video.

MLVLite compressed + sound + lens metadata works
crop recording also works.


dfort

Hum--maybe it is because I'm testing it on the EOSM?

Lars Steenhoff

I don't normally use silent pictures, so I did not test for that.
Video mlvlite functions seems to work normal.

aprofiti

Quote from: dfort on July 16, 2018, 03:04:18 PM
Resolving this issue will probably get us a step closer to getting ELNS (extended lens metadata?) working on the crop_rec_4k branch.
Using the revised version of ELNS block, I can't get silent pictures works, not simple neither FRSP.
Mlv video files are created and can be viewed from mlv_play when produced with mlv_rec.
No ELNS metadata is written to file....

Silent pictures code prints that error when block size doesn't match or can't write MLVI header. As example It get called from those line:

if (FIO_WriteFile(save_file, &elns_hdr, elns_hdr.blockSize) != (int)elns_hdr.blockSize) goto write_error;
or
if (!silent_write_mlv_chunk_headers(save_file, raw_info, 0)) goto write_error;


Yesterday I got an assert triggered with mlv_rec:

ASSERT((current_hdr->blockSize > 0) && (current_hdr->blockSize < 0x20000000));


I suspect that is an issues when calculating or checking block/header size... I remember that some time block size was printed as a negative number for some unknown reason...

Lars Steenhoff

Tested MLV rec, and this does not work.

The screen displays :
FAILED 'cr'. queued : failed

aprofiti

if I comment this line of codee:

if (FIO_WriteFile(save_file, &elns_hdr, elns_hdr.blockSize) != (int)elns_hdr.blockSize) goto write_error;     //silent.c
if (FIO_WriteFile(f, &elns_hdr, elns_hdr.blockSize) != (int)elns_hdr.blockSize) return 0;                     //mlv_lite.c

I get silent and mlv_lite to works, I can review mlv from pc and no ELNS block inside

Don't know why mlv_rec is not working anymore...

There should be something wrong with this code block:

void mlv_fill_elns(mlv_elns_hdr_t *hdr, uint64_t start_timestamp){
    /* Calculate total block length: header + fixed data + variable lensName string */
    int string_length = strlen(lens_info.name);
    int block_length = (string_length + sizeof(mlv_elns_hdr_t) + 3) & ~3;                    //Maybe this is causing troubles

    mlv_elns_hdr_t *header = malloc(block_length);

    /* prepare header */
    mlv_set_type((mlv_hdr_t *)header, "ELNS");
    mlv_set_timestamp((mlv_hdr_t *)header, start_timestamp);
    header->blockSize = block_length;
    header->length = string_length;

    header->focalLengthMin = lens_info.lens_focal_min;
    header->focalLengthMax = lens_info.lens_focal_max;
    header->apertureMin = RAW2VALUE(aperture, lens_info.raw_aperture_min) / 10.0;
    header->apertureMax = RAW2VALUE(aperture, lens_info.raw_aperture_max) / 10.0;
    header->version = lens_info.lens_version;
    header->extenderInfo = lens_info.lens_extender;
    header->capabilities = lens_info.lens_capabilities;
    header->chipped = lens_info.lens_exists;

    /* Store lensName string at the end of mlv_elns_hdr_t */
    char *lens_hdr_payload = (char *)&header[1];                  //Is this ok? Doesn't return a pointer right after header ending?
    strcpy(lens_hdr_payload, lens_info.name);                     //Will overwrite wrong memory region if above pointer is wrong

    /* update block with new values */
    hdr = header;
}

dfort

@aprofiti - confirmed your findings but am at a loss how to fix it.

lens_info.name is used for both mlv_fill_lens and mlv_fill_elns. Isn't there some sort of confusion there?

Lars Steenhoff

I made a 5dMk3 firmware 1.2.3 build for testing:

https://bitbucket.org/larssteenhoff/magic-lantern-build/downloads/magiclantern-Nightly.2018Jul23.5D3123.zip

It has mlv lite + snd + lens info

(I removed the older mlv rec module from the zip so it can't be used because it's broken )

Build from dfort branch
https://bitbucket.org/daniel_fort/magic-lantern/branch/crop_rec_4k_ELNS

I changed one timing in src/shoot.c to do a quicker 10x zoom on half shutter.  Because I don't use an auto focus lens I prefer to use half shutter for focus check.

             #ifdef CONFIG_ZOOM_HALFSHUTTER_UILOCK
-            msleep(500);
+            msleep(100);


14 bit compressed with sound works and it gives the lens info to photoshop for automatic lens corrections.

(Except the Zeiss Makro-Planar T* 2/100 ZF.2)

dfort

Quote from: Lars Steenhoff on July 23, 2018, 12:40:40 PM
14 bit compressed with sound works and it gives the lens info to photoshop for automatic lens corrections.

(Except the Zeiss Makro-Planar T* 2/100 ZF.2)

The only thing that doesn't seem to be working is the ELNS metadata which will take care of that. To find out why we need ELNS go back to Reply #166 and start reading from there. The Zeiss Makro-Planar T* 2/100 ZF.2 lens name shows properly on an XMP file (when shooting CR2 still frames) but it isn't getting into the MLV metadata. ELNS does work on the manual_lens_info branch but there are considerable difference between these branches.

My tests on the EOSM are showing either no LENS metadata (when the lens is changed with the camera turned on):

Block: LENS
  Offset: 0x00000184
  Number: 5
    Size: 96
    Time: 0.839000 ms
     Name:        ''
     Serial:      '000000000' (no valid S/N)
     Focal Len:   50 mm
     Focus Dist:  0 mm
     Aperture:    f/0.00
     IS Mode:     0
     AF Mode:     3
     Lens ID:     0x00000000
     Flags:       0x00000000


or LENS name with some padded zeros (when starting the camera with a non-chipped lens mounted):

Block: LENS
  Offset: 0x00000184
  Number: 5
    Size: 96
    Time: 0.817000 ms
     Name:        'Zeiss Makro-Planar T* 2/100 ZF.2000000000'
     Serial:      '000000000' (no valid S/N)
     Focal Len:   100 mm
     Focus Dist:  0 mm
     Aperture:    f/2.00
     IS Mode:     0
     AF Mode:     3
     Lens ID:     0x00000000
     Flags:       0x00000000


in all cases, there is no ELNS block in the MLV files.

Lars Steenhoff

Yes I also go the padded zero's

Still I'm very happy how far we got,  :)

I can now at least film compressed and save half the storage and still have sound and info for the lenses that I have ( don't have the 100 makro, but wanted to see if the script works )


dfort

This is how it is supposed to work--using the manual_lens_info branch.

Block: LENS
  Offset: 0x00000184
  Number: 5
    Size: 96
    Time: 0.818000 ms
     Name:        'Zeiss Makro-Planar T* 2/100 ZF.'
     Serial:      '000000000' (no valid S/N)
     Focal Len:   100 mm
     Focus Dist:  0 mm
     Aperture:    f/2.00
     IS Mode:     0
     AF Mode:     3
     Lens ID:     0x00000000
     Flags:       0x00000000
Block: ELNS
  Offset: 0x000001e4
  Number: 6
    Size: 95
    Time: 0.838000 ms
     Name:                'Zeiss Makro-Planar T* 2/100 ZF.2'
     Focal Length Min:    0 mm
     Focal Length Max:    0 mm
     Aperture Min:        f/0.00
     Aperture Max:        f/0.00
     Version:             0
     Extender Info:       0x00
     Capabilities:        0x00
     Chipped:             0x00


Uh oh, just discovered a problem. Extracting DNG files using "mlv_dump --dng" and looking at the DNG metadata with exiftool:

Lens Model                      : Zeiss Makro-Planar T* 2/100 ZF.


If there is an ELNS block mlv_dump should be using it instead of the LENS block. Also tried it with MLVFS and found the same issue.

g3gg0

ive just seen that there is some again fixed-length implementaition in manual_lens_info_64byte branch and manual_lens_info.

is there any tool yet that processes the ELNS block?
i still want the name to be dynamic - just like some other blocks are.
Help us with datasheets - Help us with register dumps
magic lantern: 1Magic9991E1eWbGvrsx186GovYCXFbppY, server expenses: [email protected]
ONLY donate for things we have done, not for things you expect!

aprofiti

Quote from: g3gg0 on July 23, 2018, 09:10:40 PM
ive just seen that there is some again fixed-length implementaition in manual_lens_info_64byte branch and manual_lens_info

We are experimenting dynamic length version on top of fake PR.
manual_lens_info_64byte was merged into manual_lens_info

Quote from: g3gg0 on July 23, 2018, 09:10:40 PM
is there any tool yet that processes the ELNS block?
i still want the name to be dynamic - just like some other blocks are.

Yes, mlv_dump from fake PR was modified to manage dynamic length with help from "length" field (which need to be removed as you said) to know how many chars to read and print.

Still need some help with the pointer of extended lens name (in mlv.c)... I think it is causing troubles like writing over wrong memory location when string copy is used

aprofiti

Good news Guys! Managed to discover what was preventing to save .mlv files some days ago and found a fix! :)

Currently elns struct is passed by value, so mlv_fill_elns will leaves struct initialised in memory, preventing FIOWriteFile to write correct datas...

Passing elns's struct argument as a double pointer and adapting related code will makes thing works:

void mlv_fill_elns(mlv_elns_hdr_t **hdr, uint64_t start_timestamp);


Tested on silent and mlv_lite; I need to see how to adapt mlv_rec and mlv_dump in next days.
@dfort Ill'try to send you a patch to test on crop_rec_4k

Quote from: dfort on July 23, 2018, 07:04:00 PM
Uh oh, just discovered a problem. Extracting DNG files using "mlv_dump --dng" and looking at the DNG metadata with exiftool:

Lens Model                      : Zeiss Makro-Planar T* 2/100 ZF.


If there is an ELNS block mlv_dump should be using it instead of the LENS block. Also tried it with MLVFS and found the same issue.

Found where this is handled in dng.c: added in dng_fill_header() function
It's easy to adapt it to use elns instead if present, but it needs to be able to see datas by adding in dng.h:

struct frame_info
{
     ....

    /* block headers */
    ....

    mlv_elns_hdr_t elns_hdr;

    ....
};

dfort

Great news!

I added the ELNS block header to dng.h and look forward to the patch.

This is needed in silent.c, mlv.h and mlv.c, right?

void mlv_fill_elns(mlv_elns_hdr_t **hdr, uint64_t start_timestamp);


aprofiti

I post the patch also here just in case someone want to try this on top of dfort's repository:

diff --git a/modules/mlv_lite/mlv_lite.c b/modules/mlv_lite/mlv_lite.c
--- a/modules/mlv_lite/mlv_lite.c
+++ b/modules/mlv_lite/mlv_lite.c
@@ -359,10 +359,10 @@
static GUARDED_BY(RawRecTask)   mlv_idnt_hdr_t idnt_hdr;
static GUARDED_BY(RawRecTask)   mlv_expo_hdr_t expo_hdr;
static GUARDED_BY(RawRecTask)   mlv_lens_hdr_t lens_hdr;
-static GUARDED_BY(RawRecTask)   mlv_elns_hdr_t elns_hdr;
static GUARDED_BY(RawRecTask)   mlv_rtci_hdr_t rtci_hdr;
static GUARDED_BY(RawRecTask)   mlv_wbal_hdr_t wbal_hdr;
-static GUARDED_BY(LiveViewTask) mlv_vidf_hdr_t vidf_hdr;
+static GUARDED_BY(LiveViewTask) mlv_vidf_hdr_t vidf_hdr;
+static GUARDED_BY(RawRecTask)   mlv_elns_hdr_t *elns_hdr;
static GUARDED_BY(RawRecTask)   uint64_t mlv_start_timestamp = 0;
        GUARDED_BY(RawRecTask)   uint32_t raw_rec_trace_ctx = TRACE_ERROR;

@@ -3048,7 +3048,7 @@
         fail |= !mlv_write_hdr(f, (mlv_hdr_t *)&idnt_hdr);
         fail |= !mlv_write_hdr(f, (mlv_hdr_t *)&expo_hdr);
         fail |= !mlv_write_hdr(f, (mlv_hdr_t *)&lens_hdr);
-        fail |= !mlv_write_hdr(f, (mlv_hdr_t *)&elns_hdr);
+        fail |= !mlv_write_hdr(f, (mlv_hdr_t *)elns_hdr);
         fail |= !mlv_write_hdr(f, (mlv_hdr_t *)&rtci_hdr);
         fail |= !mlv_write_hdr(f, (mlv_hdr_t *)&wbal_hdr);
         fail |= mlv_write_vers_blocks(f, mlv_start_timestamp);
diff --git a/modules/mlv_rec/dng/dng.c b/modules/mlv_rec/dng/dng.c
--- a/modules/mlv_rec/dng/dng.c
+++ b/modules/mlv_rec/dng/dng.c
@@ -669,7 +669,12 @@
             {tcBaselineExposureOffset,      ttSRational,RATIONAL_ENTRY2(0, 1, header, &data_offset)},
             {tcImageDescription,            ttAscii,    STRING_ENTRY(frame_info->info_str, header, &data_offset)},
         };
-       
+
+        /* Use Lens Model from ELNS Block if present */
+        char* lens = (frame_info->elns_str != NULL)
+              && (strlen(frame_info->elns_str) > strlen(frame_info->lens_hdr.lensName))
+              ? frame_info->elns_str : frame_info->lens_hdr.lensName;
+
         struct directory_entry EXIF_IFD[EXIF_IFD_COUNT] =
         {
             {tcExposureTime,                ttRational, RATIONAL_ENTRY2((int32_t)frame_info->expo_hdr.shutterValue/1000, 1000, header, &data_offset)},
@@ -682,7 +687,7 @@
             {tcFocalPlaneXResolutionExif,   ttRational, RATIONAL_ENTRY(focal_resolution_x, header, &data_offset, 2)},
             {tcFocalPlaneYResolutionExif,   ttRational, RATIONAL_ENTRY(focal_resolution_y, header, &data_offset, 2)},
             {tcFocalPlaneResolutionUnitExif,ttShort,    1,      camera_id[current_cam].focal_unit}, //inches
-            {tcLensModelExif,               ttAscii,    STRING_ENTRY((char*)frame_info->lens_hdr.lensName, header, &data_offset)},
+            {tcLensModelExif,               ttAscii,    STRING_ENTRY(lens, header, &data_offset)},
         };
         
         /* do not put image description, if the length is zero */
diff --git a/modules/mlv_rec/dng/dng.h b/modules/mlv_rec/dng/dng.h
--- a/modules/mlv_rec/dng/dng.h
+++ b/modules/mlv_rec/dng/dng.h
@@ -76,8 +76,8 @@
     mlv_rawc_hdr_t rawc_hdr;
     mlv_expo_hdr_t expo_hdr;
     mlv_lens_hdr_t lens_hdr;
-    mlv_elns_hdr_t elns_hdr;
     mlv_wbal_hdr_t wbal_hdr;
+    char *elns_str;
     char *info_str;
};

diff --git a/modules/mlv_rec/mlv.c b/modules/mlv_rec/mlv.c
--- a/modules/mlv_rec/mlv.c
+++ b/modules/mlv_rec/mlv.c
@@ -62,7 +62,7 @@
     strncpy((char *)hdr->lensSerial, buf, 32);
}

-void mlv_fill_elns(mlv_elns_hdr_t *hdr, uint64_t start_timestamp)
+void mlv_fill_elns(mlv_elns_hdr_t **hdr, uint64_t start_timestamp)
{
     /* Calculate total block length: header + fixed data + variable lensName string */
     int string_length = strlen(lens_info.name);
@@ -73,6 +73,7 @@
     mlv_set_type((mlv_hdr_t *)header, "ELNS");
     mlv_set_timestamp((mlv_hdr_t *)header, start_timestamp);
     header->blockSize = block_length;
+    /* Fill ELNS data */
     header->focalLengthMin = lens_info.lens_focal_min;
     header->focalLengthMax = lens_info.lens_focal_max;
     header->apertureMin = RAW2VALUE(aperture, lens_info.raw_aperture_min) / 10.0;
@@ -84,10 +85,10 @@

     /* Store lensName string at the end of mlv_elns_hdr_t */
     char *lens_hdr_payload = (char *)&header[1];
-    strcpy(lens_hdr_payload, lens_info.name);
+    snprintf(lens_hdr_payload, string_length + 1, "%s", lens_info.name);

     /* update block with new values */
-    hdr = header;
+    *hdr = header;
}

void mlv_fill_wbal(mlv_wbal_hdr_t *hdr, uint64_t start_timestamp)
diff --git a/modules/mlv_rec/mlv.h b/modules/mlv_rec/mlv.h
--- a/modules/mlv_rec/mlv.h
+++ b/modules/mlv_rec/mlv.h
@@ -176,9 +176,7 @@
     uint8_t     extenderInfo;       /* extender information, if provided by camera       */
     uint8_t     capabilities;       /* capability information, if provided by camera     */
     uint8_t     chipped;            /* when not zero, lens is communicating with camera  */
-    uint8_t     length;             /* to allow lens with name longer than 32byte        */
  /* uint8_t     lensName[variable];    full lens string, null terminated                 */
-    // TODO: Review Specs
}  mlv_elns_hdr_t;

typedef struct {
@@ -304,7 +302,7 @@
void mlv_fill_rtci(mlv_rtci_hdr_t *hdr, uint64_t start_timestamp);
void mlv_fill_expo(mlv_expo_hdr_t *hdr, uint64_t start_timestamp);
void mlv_fill_lens(mlv_lens_hdr_t *hdr, uint64_t start_timestamp);
-void mlv_fill_elns(mlv_elns_hdr_t *hdr, uint64_t start_timestamp);
+void mlv_fill_elns(mlv_elns_hdr_t **hdr, uint64_t start_timestamp);
void mlv_fill_idnt(mlv_idnt_hdr_t *hdr, uint64_t start_timestamp);
void mlv_fill_wbal(mlv_wbal_hdr_t *hdr, uint64_t start_timestamp);
void mlv_fill_styl(mlv_styl_hdr_t *hdr, uint64_t start_timestamp);
diff --git a/modules/mlv_rec/mlv_dump.c b/modules/mlv_rec/mlv_dump.c
--- a/modules/mlv_rec/mlv_dump.c
+++ b/modules/mlv_rec/mlv_dump.c
@@ -1941,7 +1941,8 @@
     memset(&rtci_info, 0x00, sizeof(mlv_rtci_hdr_t));
     memset(&rawc_info, 0x00, sizeof(mlv_rawc_hdr_t));
     memset(&main_header, 0x00, sizeof(mlv_file_hdr_t));
-
+
+    char elns_string[1024] = "";
     char info_string[1024] = "";

     /* this table contains the XREF chunk read from idx file, if existing */
@@ -3468,7 +3469,8 @@
                             frame_info.expo_hdr             = expo_info;
                             frame_info.lens_hdr             = lens_info;
                             frame_info.wbal_hdr             = wbal_info;
-                            frame_info.rawc_hdr             = rawc_info;
+                            frame_info.rawc_hdr             = rawc_info;
+                            frame_info.elns_str             = elns_string;
                             frame_info.info_str             = info_string;
                             frame_info.rawi_hdr.xRes        = lv_rec_footer.xRes;
                             frame_info.rawi_hdr.yRes        = lv_rec_footer.yRes;
@@ -3780,30 +3782,24 @@
             {
                 mlv_elns_hdr_t block_hdr = *(mlv_elns_hdr_t *)mlv_block;

-                /* get the string length and malloc a buffer for that string */
-                int str_length = block_hdr.length;
-
-                if(str_length)
+                void *payload = BYTE_OFFSET(mlv_block, sizeof(mlv_elns_hdr_t));
+                int str_length = MIN(block_hdr.blockSize - sizeof(block_hdr), sizeof(elns_string) - 1);
+
+                /* Fill lens model data for DNG processing */
+                strncpy(elns_string, payload, str_length);
+                elns_string[str_length] = '\000';
+
+                if(verbose)
                 {
-                    void *payload = BYTE_OFFSET(mlv_block, sizeof(mlv_elns_hdr_t) - 1);
-                    char *buf = malloc(str_length + 1);
-
-                    strncpy(buf, payload, str_length);
-                    buf[str_length] = '\000';
-
-                    if(verbose)
-                    {
-                        print_msg(MSG_INFO, "     Name:                '%s'\n", buf);
-                        print_msg(MSG_INFO, "     Focal Length Min:    %d mm\n", elns_info.focalLengthMin);
-                        print_msg(MSG_INFO, "     Focal Length Max:    %d mm\n", elns_info.focalLengthMax);
-                        print_msg(MSG_INFO, "     Aperture Min:        f/%.2f\n", (double)elns_info.apertureMin);
-                        print_msg(MSG_INFO, "     Aperture Max:        f/%.2f\n", (double)elns_info.apertureMax);
-                        print_msg(MSG_INFO, "     Version:             %d\n", elns_info.version);
-                        print_msg(MSG_INFO, "     Extender Info:       0x%02X\n", elns_info.extenderInfo);
-                        print_msg(MSG_INFO, "     Capabilities:        0x%02X\n", elns_info.capabilities);
-                        print_msg(MSG_INFO, "     Chipped:             0x%02X\n", elns_info.chipped);
-                    }
-                    free(buf);
+                    print_msg(MSG_INFO, "     Name:                '%s'\n", payload);
+                    print_msg(MSG_INFO, "     Focal Length Min:    %d mm\n", block_hdr.focalLengthMin);
+                    print_msg(MSG_INFO, "     Focal Length Max:    %d mm\n", block_hdr.focalLengthMax);
+                    print_msg(MSG_INFO, "     Aperture Min:        f/%.2f\n", (double)block_hdr.apertureMin);
+                    print_msg(MSG_INFO, "     Aperture Max:        f/%.2f\n", (double)block_hdr.apertureMax);
+                    print_msg(MSG_INFO, "     Version:             %d\n", block_hdr.version);
+                    print_msg(MSG_INFO, "     Extender Info:       0x%02X\n", block_hdr.extenderInfo);
+                    print_msg(MSG_INFO, "     Capabilities:        0x%02X\n", block_hdr.capabilities);
+                    print_msg(MSG_INFO, "     Chipped:             0x%02X\n", block_hdr.chipped);
                 }
             }
             else if(!memcmp(mlv_block->blockType, "ELVL", 4))
diff --git a/modules/mlv_rec/mlv_rec.c b/modules/mlv_rec/mlv_rec.c
--- a/modules/mlv_rec/mlv_rec.c
+++ b/modules/mlv_rec/mlv_rec.c
@@ -200,9 +200,9 @@

static mlv_expo_hdr_t last_expo_hdr;
static mlv_lens_hdr_t last_lens_hdr;
-static mlv_elns_hdr_t last_elns_hdr;
static mlv_wbal_hdr_t last_wbal_hdr;
-static mlv_styl_hdr_t last_styl_hdr;
+static mlv_styl_hdr_t last_styl_hdr;
+static mlv_elns_hdr_t *last_elns_hdr;


/* for debugging */
@@ -1535,13 +1535,13 @@
             trace_write(raw_rec_trace_ctx, "[polling_cbr] queueing INFO blocks");
             mlv_expo_hdr_t *expo_hdr = malloc(sizeof(mlv_expo_hdr_t));
             mlv_lens_hdr_t *lens_hdr = malloc(sizeof(mlv_lens_hdr_t));
+            mlv_wbal_hdr_t *wbal_hdr = malloc(sizeof(mlv_wbal_hdr_t));
             mlv_elns_hdr_t *elns_hdr = malloc(sizeof(mlv_elns_hdr_t));
-            mlv_wbal_hdr_t *wbal_hdr = malloc(sizeof(mlv_wbal_hdr_t));

             mlv_fill_expo(expo_hdr, mlv_start_timestamp);
             mlv_fill_lens(lens_hdr, mlv_start_timestamp);
-            mlv_fill_elns(elns_hdr, mlv_start_timestamp);
-            mlv_fill_wbal(wbal_hdr, mlv_start_timestamp);
+            mlv_fill_wbal(wbal_hdr, mlv_start_timestamp);
+            mlv_fill_elns(&elns_hdr, mlv_start_timestamp);

             msg_queue_post(mlv_block_queue, (uint32_t) expo_hdr);
             msg_queue_post(mlv_block_queue, (uint32_t) lens_hdr);
@@ -2674,10 +2674,10 @@
         mlv_rtci_hdr_t rtci_hdr;
         mlv_expo_hdr_t expo_hdr;
         mlv_lens_hdr_t lens_hdr;
-        mlv_elns_hdr_t elns_hdr;
         mlv_idnt_hdr_t idnt_hdr;
         mlv_wbal_hdr_t wbal_hdr;
         mlv_styl_hdr_t styl_hdr;
+        mlv_elns_hdr_t *elns_hdr;

         mlv_fill_rtci(&rtci_hdr, mlv_start_timestamp);
         mlv_fill_expo(&expo_hdr, mlv_start_timestamp);
@@ -2694,12 +2694,12 @@
         idnt_hdr.timestamp = 4;
         wbal_hdr.timestamp = 5;
         styl_hdr.timestamp = 6;
-        elns_hdr.timestamp = 7;
+        elns_hdr->timestamp = 7;

         mlv_write_hdr(f, (mlv_hdr_t *)&rtci_hdr);
         mlv_write_hdr(f, (mlv_hdr_t *)&expo_hdr);
         mlv_write_hdr(f, (mlv_hdr_t *)&lens_hdr);
-        mlv_write_hdr(f, (mlv_hdr_t *)&elns_hdr);
+        mlv_write_hdr(f, (mlv_hdr_t *)elns_hdr);
         mlv_write_hdr(f, (mlv_hdr_t *)&idnt_hdr);
         mlv_write_hdr(f, (mlv_hdr_t *)&wbal_hdr);
         mlv_write_hdr(f, (mlv_hdr_t *)&styl_hdr);
@@ -3211,7 +3211,7 @@

         mlv_expo_hdr_t old_expo = last_expo_hdr;
         mlv_lens_hdr_t old_lens = last_lens_hdr;
-        mlv_elns_hdr_t old_elns = last_elns_hdr;
+        mlv_elns_hdr_t *old_elns = last_elns_hdr;

         mlv_fill_expo(&last_expo_hdr, mlv_start_timestamp);
         mlv_fill_lens(&last_lens_hdr, mlv_start_timestamp);
@@ -3220,7 +3220,7 @@
         /* update timestamp for comparing content changes */
         old_expo.timestamp = last_expo_hdr.timestamp;
         old_lens.timestamp = last_lens_hdr.timestamp;
-        old_elns.timestamp = last_elns_hdr.timestamp;
+        old_elns->timestamp = last_elns_hdr->timestamp;

         /* write new state if something changed */
         if(memcmp(&last_expo_hdr, &old_expo, sizeof(mlv_expo_hdr_t)))
@@ -3240,8 +3240,8 @@
         /* write new state if something changed */
         if(memcmp(&last_elns_hdr, &old_elns, sizeof(mlv_elns_hdr_t)))
         {
-            mlv_hdr_t *hdr = malloc(sizeof(mlv_elns_hdr_t));
-            memcpy(hdr, &last_elns_hdr, sizeof(mlv_elns_hdr_t));
+            mlv_hdr_t *hdr = malloc(last_elns_hdr->blockSize);
+            memcpy(hdr, last_elns_hdr, last_elns_hdr->blockSize);
             msg_queue_post(mlv_block_queue, (uint32_t) hdr);
         }
     }
diff --git a/modules/silent/silent.c b/modules/silent/silent.c
--- a/modules/silent/silent.c
+++ b/modules/silent/silent.c
@@ -28,10 +28,10 @@
extern WEAK_FUNC(ret_0) void mlv_fill_rtci(mlv_rtci_hdr_t *hdr, uint64_t start_timestamp);
extern WEAK_FUNC(ret_0) void mlv_fill_expo(mlv_expo_hdr_t *hdr, uint64_t start_timestamp);
extern WEAK_FUNC(ret_0) void mlv_fill_lens(mlv_lens_hdr_t *hdr, uint64_t start_timestamp);
-extern WEAK_FUNC(ret_0) void mlv_fill_elns(mlv_elns_hdr_t *hdr, uint64_t start_timestamp);
extern WEAK_FUNC(ret_0) void mlv_fill_idnt(mlv_idnt_hdr_t *hdr, uint64_t start_timestamp);
extern WEAK_FUNC(ret_0) void mlv_fill_wbal(mlv_wbal_hdr_t *hdr, uint64_t start_timestamp);
extern WEAK_FUNC(ret_0) void mlv_fill_styl(mlv_styl_hdr_t *hdr, uint64_t start_timestamp);
+extern WEAK_FUNC(ret_0) void mlv_fill_elns(mlv_elns_hdr_t **hdr, uint64_t start_timestamp);
extern WEAK_FUNC(ret_0_long) uint64_t mlv_generate_guid();
extern WEAK_FUNC(ret_0) void mlv_init_fileheader(mlv_file_hdr_t *hdr);
extern WEAK_FUNC(ret_0) void mlv_set_type(mlv_hdr_t *hdr, char *type);
@@ -366,11 +366,11 @@
     mlv_rtci_hdr_t rtci_hdr;
     mlv_expo_hdr_t expo_hdr;
     mlv_lens_hdr_t lens_hdr;
-    mlv_elns_hdr_t elns_hdr;
     mlv_idnt_hdr_t idnt_hdr;
     mlv_wbal_hdr_t wbal_hdr;
     mlv_styl_hdr_t styl_hdr;
     mlv_vidf_hdr_t vidf_hdr;
+    mlv_elns_hdr_t *elns_hdr;
     FILE* save_file = NULL;   
     
     /* default case: use last filename */
@@ -489,7 +489,7 @@
     if (FIO_WriteFile(save_file, &rtci_hdr, rtci_hdr.blockSize) != (int)rtci_hdr.blockSize) goto write_error;
     if (FIO_WriteFile(save_file, &expo_hdr, expo_hdr.blockSize) != (int)expo_hdr.blockSize) goto write_error;
     if (FIO_WriteFile(save_file, &lens_hdr, lens_hdr.blockSize) != (int)lens_hdr.blockSize) goto write_error;
-    if (FIO_WriteFile(save_file, &elns_hdr, elns_hdr.blockSize) != (int)elns_hdr.blockSize) goto write_error;
+    if (FIO_WriteFile(save_file, elns_hdr, elns_hdr->blockSize) != (int)elns_hdr->blockSize) goto write_error;

     memset(&vidf_hdr, 0, sizeof(mlv_vidf_hdr_t));
     mlv_set_type((mlv_hdr_t *)&vidf_hdr, "VIDF");
diff --git a/scripts/lib/config.lua b/scripts/lib/config.lua
--- a/scripts/lib/config.lua
+++ b/scripts/lib/config.lua
@@ -79,8 +79,26 @@

end

+-- Load config from file if exists, otherwise create a new table in memory
local create_internal = function(default,thisfile)
+  local filename = string.format("%s%s.lcf", dryos.config_dir.path,thisfile)
+  local cfg = config.findConfig(filename)

+  if cfg == nil then
+    -- Create a config from scratch
+    cfg = {}
+    cfg.filename = filename
+    cfg.default = default -- TODO: Replicate .data's structure
+    cfg.data = {}
+    -- check for existing .cfg to load
+    setmetatable(cfg,config)
+    -- load previus config from file if available
+    cfg.data = cfg:load()
+    -- add to data structure
+    table.insert(config.configs,cfg)
+  end
+
+  return cfg
end

local function recursiveLoad(m,cfg)
@@ -108,33 +126,17 @@
]]
function config.create(default)
   local short_name = string.match(debug.getinfo(2,"S").short_src,"/([^/%.]+)%.[^/%.]+$")
-  local filename = string.format("%s%s.lcf", dryos.config_dir.path,short_name)
+  local cfg = create_internal(default,short_name)

-  local cfg = config.findConfig(filename)
-  if cfg ~= nil then
-    -- Append config data
-    for k,v in pairs(default) do
-      cfg.data[k] = v
-    end
-  else
-    -- Create a config from scratch
-    cfg = {}
-    cfg.filename = filename
-    cfg.default = default -- TODO: Replicate .data's structure
-    cfg.data = {}
-    -- check for existing .cfg to load
-    setmetatable(cfg,config)
-    cfg.data = cfg:load()
-    if cfg.data == nil then
-      -- Create a config from scratch
-      for k,v in pairs(default) do
-        cfg.data[k] = v
-      end
-    end
-    table.insert(config.configs,cfg)
-   end
+  -- Append config data
+  for k,v in pairs(default) do
+    cfg.default[k] = v
+    cfg.data[k] = v
+  end

-   return cfg
+  --  cfg.default = default -- TODO: Replicate .data's structure
+
+  return cfg
end

--[[---------------------------------------------------------------------------
@@ -158,37 +160,18 @@
     insertMenu(default,m)

     local short_name = string.match(debug.getinfo(2,"S").short_src,"/([^/%.]+)%.[^/%.]+$")
-    local filename = string.format("%s%s.lcf", dryos.config_dir.path,short_name)
+    local cfg = create_internal(default,short_name)

-    local cfg = config.findConfig(filename)
-    if cfg ~= nil then
-      -- Already present in config.configs, append menu
-      if cfg.data[m.name] ~= nil then
-        -- Avoid overwriting values when loading config form .cfg
-        cfg.data[m.name].menu = m
-        recursiveLoad(m,cfg.data[m.name])
-      else
-        insertMenu(cfg.data,m)
-      end
+    if cfg.data[m.name] == nil then
+      -- Create a config for menu from scratch
+      insertMenu(cfg.data,m)
     else
-      -- Create a config from scratch
-      cfg = {}
-      cfg.filename = filename
-      cfg.default = default -- TODO: Replicate .data's structure
-      cfg.data = {}
-      -- check for existing .cfg to load
-      setmetatable(cfg,config)
-      cfg.data = cfg:load()
-      if cfg.data == nil then
-        -- Create a config from scratch
-        insertMenu(cfg.data,m)
-      else
-        -- load values to menu
-        cfg.data[m.name].menu = m
-        recursiveLoad(m,cfg.data[m.name])
-      end
-      table.insert(config.configs,cfg)
-     end
+      -- Already present in config.configs, load values to menu from config
+      cfg.data[m.name].menu = m
+      recursiveLoad(m,cfg.data[m.name])
+    end
+
+    --    cfg.default = default -- TODO: Replicate .data's structure

     return cfg.data[m.name]
end
@@ -213,9 +196,6 @@
@function saving
]]
function config:saving()
-local short_name = string.match(debug.getinfo(2,"S").short_src,"/([^/%.]+)%.[^/%.]+$")
-local filename = string.format("%s%s.lcf", dryos.config_dir.path,short_name)
-
   -- Copy values of each menu
   for k,v in pairs(self.data) do
     -- k -> A table representing a menu entry or a single entry of a simple config
@@ -238,14 +218,15 @@
]]
function config:save()
     local f = io.open(self.filename,"w")
+    assert(f ~= nil, "Could not save config: "..self.filename)
+    -- Serialize data into a loadable format
     f:write("return ")
-    assert(f ~= nil, "Could not save config: "..self.filename)
-    config.serialize(f,self.data)
+    config.serialize(f,self.data,1)
     f:close()
end

--private
-function config.serialize(f,o)
+function config.serialize(f,o,lvl)
     if type(o) == "number" or type(o) == "boolean" then
         f:write(tostring(o))
     elseif type(o) == "string" then
@@ -254,13 +235,17 @@
         f:write("{\n")
         for k,v in pairs(o) do
           if k ~= "menu" then
-            f:write("\t[")
-            config.serialize(f,k)
+            -- Indent starting line
+            f:write(string.rep("\t", lvl))
+            f:write("[")
+            config.serialize(f,k,lvl+1)
             f:write("] = ")
-            config.serialize(f,v)
+            config.serialize(f,v,lvl+1)
             f:write(",\n")
           end
         end
+        -- Indent closing bracket
+        f:write(string.rep("\t", lvl-1))
         f:write("}")
     else
         --something we don't know how to serialize, just skip it


Tested those changes in manual_lens_info and adapted for crop_rec_4k.
Any Feedback is welcome :)

dfort

Applied the patch and it is looking much better. The ELNS Block is working but the trailing zeros in the LENS Block looks like it is still an issue.

Block: LENS
  Offset: 0x00000184
  Number: 5
    Size: 96
    Time: 0.803000 ms
     Name:        'Samyang 8mm f/3.5 UMC Fish-Eye C000000000'
     Serial:      '000000000' (no valid S/N)
     Focal Len:   8 mm
     Focus Dist:  0 mm
     Aperture:    f/3.50
     IS Mode:     0
     AF Mode:     3
     Lens ID:     0x00000000
     Flags:       0x00000000
Block: ELNS
  Offset: 0x000001e4
  Number: 6
    Size: 68
    Time: 0.988000 ms
     Name:                'Samyang 8mm f/3.5 UMC Fish-Eye CS II'
     Focal Length Min:    0 mm
     Focal Length Max:    0 mm
     Aperture Min:        f/0.00
     Aperture Max:        f/0.00
     Version:             0
     Extender Info:       0x00
     Capabilities:        0x00
     Chipped:             0x01


When a CPU lens is attached it is fine:

Block: LENS
  Offset: 0x00000184
  Number: 5
    Size: 96
    Time: 15.291000 ms
     Name:        'EF-M11-22mm f/4-5.6 IS STM'
     Serial:      '000004904' (18692)
     Focal Len:   16 mm
     Focus Dist:  65535 mm
     Aperture:    f/8.00
     IS Mode:     0
     AF Mode:     0
     Lens ID:     0x00001033
     Flags:       0x00000000
Block: ELNS
  Offset: 0x000001e4
  Number: 6
    Size: 60
    Time: 15.476000 ms
     Name:                'EF-M11-22mm f/4-5.6 IS STM'
     Focal Length Min:    11 mm
     Focal Length Max:    22 mm
     Aperture Min:        f/4.00
     Aperture Max:        f/26.00
     Version:             65536
     Extender Info:       0xFF
     Capabilities:        0x37
     Chipped:             0x01


aprofiti

I think because it's printing serial number at the end of lens name, due to a missing termination character of the string... (present after first 32 char)

Two option here:
1) hardening string with a termination character in mlv.c:

diff --git a/modules/mlv_rec/mlv.c b/modules/mlv_rec/mlv.c
--- a/modules/mlv_rec/mlv.c
+++ b/modules/mlv_rec/mlv.c
@@ -58,7 +58,7 @@
     char buf[33];
     snprintf(buf, sizeof(buf), "%X%08X", (uint32_t) (lens_info.lens_serial >> 32), (uint32_t)(lens_info.lens_serial & 0xFFFFFFFF));
     
-    strncpy((char *)hdr->lensName, lens_info.name, 32);
+    snprintf((char *)hdr->lensName, 32, "%s", lens_info.name);
     strncpy((char *)hdr->lensSerial, buf, 32);
}

As a drawback we will loose a char of the string, making space for a 31 char string without using ELNS block

2) modify mlv_dump to copy first 32 character of the string in a "buffer" and then print it instead of lens_info.lensName

diff --git a/modules/mlv_rec/mlv_dump.c b/modules/mlv_rec/mlv_dump.c
--- a/modules/mlv_rec/mlv_dump.c
+++ b/modules/mlv_rec/mlv_dump.c
@@ -3670,18 +3670,22 @@

                 if(verbose)
                 {
+                    /* Ensure lens name is null trminated to avoid printing serial number */
+                    char name[33];
+                    snprintf(name, sizeof(name), "%s", lens_info.lensName);
+
                     uint64_t serial = 0;
                     char serial_str[64];
                     char *end;
-                   
+
                     strcpy(serial_str, "no valid S/N");
                     serial = strtoull((char *)lens_info.lensSerial, &end, 16);
                     if (serial && !*end)
                     {
                         sprintf(serial_str, "%"PRIu64, serial);
                     }
-                   
-                    print_msg(MSG_INFO, "     Name:        '%s'\n", lens_info.lensName);
+
+                    print_msg(MSG_INFO, "     Name:        '%s'\n", name);
                     print_msg(MSG_INFO, "     Serial:      '%s' (%s)\n", lens_info.lensSerial, serial_str);
                     print_msg(MSG_INFO, "     Focal Len:   %d mm\n", lens_info.focalLength);
                     print_msg(MSG_INFO, "     Focus Dist:  %d mm\n", lens_info.focalDist);


Can cause similar issue when processing with other tools if they don't take care of it

dfort

Quote from: aprofiti on August 08, 2018, 03:32:58 AM
As a drawback we will loose a char of the string, making space for a 31 char string without using ELNS block

Isn't that what we already have? Check out Reply #167 from @dmilligan and the following couple of posts.

aprofiti

LENS block processing is still untouched from some times (both mlv.c and mlv_dump.c); the only changes was about serial number fix made by g3gg0.

I think it was unoticed  until now or maybe it showed up after increasing size of lens_info.name to 64 byte.

In mlv 2.0 specifications, lens name is a optional null terminated string, so maybe option number 2 is to be preferred to avoid interference with other processing tools?

g3gg0

Quote from: aprofiti on August 08, 2018, 11:05:19 AM
In mlv 2.0 specifications, lens name is a optional null terminated string, so maybe option number 2 is to be preferred to avoid interference with other processing tools?

absolutely. this is my recommendation.
in MLV strings dont *have to* be null terminated as you pointed out correctly.
Help us with datasheets - Help us with register dumps
magic lantern: 1Magic9991E1eWbGvrsx186GovYCXFbppY, server expenses: [email protected]
ONLY donate for things we have done, not for things you expect!

dfort

Applied the second option, patched mlv_dump:

Block: LENS
  Offset: 0x00000184
  Number: 5
    Size: 96
    Time: 0.803000 ms
     Name:        'Samyang 8mm f/3.5 UMC Fish-Eye C'
     Serial:      '000000000' (no valid S/N)
     Focal Len:   8 mm
     Focus Dist:  0 mm
     Aperture:    f/3.50
     IS Mode:     0
     AF Mode:     3
     Lens ID:     0x00000000
     Flags:       0x00000000
Block: ELNS
  Offset: 0x000001e4
  Number: 6
    Size: 68
    Time: 0.988000 ms
     Name:                'Samyang 8mm f/3.5 UMC Fish-Eye CS II'
     Focal Length Min:    0 mm
     Focal Length Max:    0 mm
     Aperture Min:        f/0.00
     Aperture Max:        f/0.00
     Version:             0
     Extender Info:       0x00
     Capabilities:        0x00
     Chipped:             0x01


In addition, we gained one character in the LENS block so the lens that started this whole extended lens name issue is showing up fine now.

Block: LENS
  Offset: 0x00000184
  Number: 5
    Size: 96
    Time: 0.812000 ms
     Name:        'Zeiss Makro-Planar T* 2/100 ZF.2'
     Serial:      '000000000' (no valid S/N)
     Focal Len:   100 mm
     Focus Dist:  0 mm
     Aperture:    f/2.00
     IS Mode:     0
     AF Mode:     3
     Lens ID:     0x00000000
     Flags:       0x00000000
Block: ELNS
  Offset: 0x000001e4
  Number: 6
    Size: 64
    Time: 0.997000 ms
     Name:                'Zeiss Makro-Planar T* 2/100 ZF.2'
     Focal Length Min:    0 mm
     Focal Length Max:    0 mm
     Aperture Min:        f/0.00
     Aperture Max:        f/0.00
     Version:             0
     Extender Info:       0x00
     Capabilities:        0x00
     Chipped:             0x01


Why? Because the LENS name is no longer null terminated.

Quote from: dmilligan on January 11, 2017, 12:51:00 PM
The last character of the string has to be the null terminator (0). So I slightly misspoke, the lens name is limited to 32 bytes (31 ascii characters).

Quote from: g3gg0 on August 08, 2018, 11:26:43 AM
in MLV strings dont *have to* be null terminated as you pointed out correctly.

@aprofiti -- our repositories seem to be out of sync because your patch didn't apply cleanly. Do you want to do the pull request or do you want me to do it? Note that we'll need to break it down into smaller commits.

https://bitbucket.org/daniel_fort/magic-lantern/pull-requests/22/elns-for-crop_rec_4k/diff

aprofiti

Quote from: dfort on August 11, 2018, 04:37:43 PM
Do you want to do the pull request or do you want me to do it?

If everything is working fine I would like to commit the "dynamic" version first on manual_lens_info (to have the original branch up to date) and then make a new PR to crop_rec_4k_mlv_snd (is this the currently bleeding edge branch? I'll need your help to review this PR). What do you think about?

Quote from: dfort on August 11, 2018, 04:37:43 PM
Note that we'll need to break it down into smaller commits.
Regarding breaking down into smaller commits, I can do this in manual_lens_inf PR; do it needs to be done also when merging to crop_rec?

Quote from: dfort on August 11, 2018, 04:37:43 PM
our repositories seem to be out of sync because your patch didn't apply cleanly.
My repo was based on top of crop_rec_4k but then we moved to the one with sound support.
I had to solve more conflicts when merged mlv_snd but due to some problems determining which diff were the most up to data, I screw it up and moved working on top of manual_lens_info adapting changes to works with you repo.

Quote from: dfort on August 11, 2018, 04:37:43 PM
https://bitbucket.org/daniel_fort/magic-lantern/pull-requests/22/elns-for-crop_rec_4k/diff
Having your repo as a reference could be useful to avoid missing something when merging to crop_rec_4k*, because differences from crop_rec_4k and crop_rec_4k_mlv_snd are not exactly clear to me.