Add new command line options to raspiraw

[Hermann-SW]

The new command line options mainly allow for high framerate video capturing into ramdisk (because SD card is too slow), up to 665fps for now.

Two options are useful for all existing modes without high framerate capturing:
--fps : allows to change framerate for each of the existing modes 1-7
--tstamps : allows to write frame capture timestamps into file for frame skip and frame delta analysis

[6by9]

A good start, but some ugly bits / things to consider.

[6by9]

You're right that modes 4&5 appear to have an error.
raspivid -w 1280 -h 720 -md 4 and raspivid -w 1280 -h 720 -md 5 should give the same FOV (with mode 5 having a slightly higher max frame rate), but they don't.
I'm looking at fixing that in the firmware now, but at first glance it appears that 0x3802 should be 0x00, and 0x3803 should be 0xF0 (240).

[6by9]

0x3814 needs to come from the struct sensor_def, same as exposure_reg, vts_reg, and gain_reg,

[6by9]

0x3815 needs to come from the struct sensor_def, same as exposure_reg, vts_reg, and gain_reg,

[6by9]

On almost all sensors vts is expressed in line periods, which you already have as sensor_mode->line_time_ns.

frame_period_nsecs = 1000000000 / cfg.fps;
n = frame_period_nsecs / sensor_mode->line_time_ns;
//write to register sensor->vts_reg

should get you to the same place in a sensor independent manner.
(I haven't checked for overflows in those calcs. You may need to use 64 bit values)

[6by9]

Do you really need to ever override this? You aren't altering the line length, and I think if you are then you're better off defining a new mode.

[6by9]

Duplicate of modReg(mode, r, 0, 7, b, EQUAL).

[6by9]

The update_regs call at line 949 has the potential to undo anything up to this point.
Probably better to alter update_regs to take in the cfg struct (as a const) and then apply all these settings there as one central point.

[6by9]

Take in a new filename again rather than squeezing it in as file 0. The user can then choose to name it as file 0, or use something more to their liking.

[6by9]

No checking the malloc succeeded -> seg fault.

Simplest solution is change line 429 to
if (cfg->write_timestamps && cfg->ptso)
or even
if (cfg->ptso)
as cfg->ptso will never get set if write_timestamps is 0.

(Lots of little mallocs like this isn't nice, but such is life. The alternative is to allocate bigger arrays to take eg 1024 timestamps, and chain those together.)

[6by9]

Thinking aloud more than anything.
To alter the crop you're currently forcing the user to specify register values and then width/height - lots of potential for errors, and I have seen it result in memory corruption (I wish I knew exactly why, as it shouldn't be possible).

Are we better off with left/right/top/bottom crop parameters that then compute the registers and width / height? I'm happy enough with a function pointer in the sensor_def to a function that works out the values rather trying to write generic code with lookup tables of registers.
hinc and vinc are the awkward ones, but again messing with those really ought to be done as a new mode.

[Hermann-SW]

Hi 6by9, thanks for your comments, I will carefully work on them and try to address all issues. I saw that you did an unrelated commit today, and learned how to update my fork from your original repo to keep up with that change. Let me now show you a (small) issue that occurred first after creating separate repo for raspiraw: pi@raspberrypi2B:~/raspiraw $ ./buildme raspiraw.c: In function ‘create_filenames’: raspiraw.c:401:10: warning: implicit declaration of function ‘asprintf’ [-Wimplicit-function-declaration] if (0 > asprintf(finalName, pattern, frame)) ^~~~~~~~ raspiraw.c: In function ‘main’: raspiraw.c:831:45: warning: implicit declaration of function ‘basename’ [-Wimplicit-function-declaration] fprintf(stdout, "\n%s Camera App %s\n\n", basename(argv[0]), VERSION_STRING); ^~~~~~~~ pi@raspberrypi2B:~/raspiraw $ Maybe that shows up for me only since I build on Raspbian and you maybe differently. I looked up man page for asprintf and it states that <stdio.h> needs to be included, but that is what you do in raspiraw.c -- it would be really cool if you can silence the two warnings somehow. Thanks for your ongoing support in multiple threads on Raspberry forum! Btw, I seem to have maxed out 640x128 (stretched from x64) at sensational 665fps, without frame skips for 8s! Officially ov5647 can do 90fps with 480p. Officially imx219 can do 180fps with 720p! (2816x1584) If linearity is kept by imx219 hardware, then maybe 2816x128 (or 1408x128) stretched from 64 can show 1.5*2*665 ~ 2000fps?!?!?!? Hermann.

[Hermann-SW]

Hi, I cannot match the formulas you provided to match the measured framerates of unmodified raspiraw. Here are your formaulas: frame_period_nsecs = 1000000000 / cfg.fps; n = frame_period_nsecs / sensor_mode->line_time_ns; //write to register sensor->vts_reg let me abbreviate: frame_period_nsecs = 1000000000 / cfg.fps vts = frame_period_nsecs / line_time_ns Rearrange 2nd: vts * line_time_ns = frame_period_nsecs With 1st: vts * line_time_ns = 1000000000 / cfg.fps resulting in this formula for fps based on vts and line_time_ns: fps = 1000000000 / (vts * line_time_ns) I did see 30fps for unmodified raspiraw mode4 and mode5, 42fps for mode6 and 60fps for mode7. Now evaluate the formula with your vts and line_time_ns values for these modes from ov5647_modes.h: $ bc -ql 1000000000/(996*23216) 43.24672916337991525024 1000000000/(870*23216) 49.51004855945562711407 1000000000/(484*31749) 65.07655996974200267646 1000000000/(484*21165) 97.61945204249179508505 These values are completely different to 30/30/42/60. So is something wrong in the formula, or in the vts/line values in struct mode_def ov5647_modes[] ? Dividing by the values measured with unmodified raspiraw does not give consistent values either ... 1000000000/(484*21165)/60 1.62699086737486325141 1000000000/(484*31749)/42 1.54944190404147625420 1000000000/(870*23216)/30 1.65033495198185423713 1000000000/(996*23216)/30 1.44155763877933050834 Hermann.

[6by9]

You're calculating VTS to be used based on the frame rate.

The vts values in the mode_def struct is the minimum that I believe the sensor will run with, and therefore the absolute maximum framerate. They are NOT the value that is expected to be set in the register set.

Taking as an example mode7, 21165 * 484 = 10243860ns, or 10.24ms. 1000000000/(21165 * 484) = absolute max of 97.6 fps. As you'd observed, mode7 can be convinced to run up to 96fps, although 95fps in reality.
The default programmed value is 0x312, or 786. 786*21165 = 16635690ns, or 60.111fps as you had observed.

Or mode0 (5MPix), 32503 * 1968 = 654365904ns, or 65.43ms. 1000000000/(32503 * 1968) = 15.633fps, when max permitted is 15fps.

vts_reg = 1000000000 / (fps * sensor_mode->line_length should work out.

[Hermann-SW]

Hi, sorry, I read your emails only just now. And I did a lot of git pushs on the weekend, so these are not on your merged branch. There are now many mode tools that allow to capture at different resolutions. Framerates from 210 to 750, see table here -- far higher than I ever had hoped for! https://github.com/Hermann-SW/raspiraw#tools-usage I forked from your branch by clicking on "Fork" in browser. Yesterday I was able to get all your new submits merged into my branch. The network diagram for your branch seems to confirm this: https://github.com/6by9/raspiraw/network From your previous comments I have addressed all but 2(hinc), 3(vinc) and 13(width/height/regs). On 13 I have the plan to completely not use --regs option. There will be --columns and --rows instead of width and height. This is partially your crop idea (left, top), but allows stretching. So with --colums and --rows you define the output format completely. But the input area depends (we will get many different 320x240 modes that way, with and without stretching), see this: Input area: cols * scale(xoddinc, xeveninc) x rows * scale(yoddinc, yeveninc) starting (left, top) scale() is function of the increments, for default values 1. Output: cols x rows frame All the needed settings will be done from cols/rows/left/top/hinc/vinc. I found the vinc/hinc registers in imx datasheet. This is plan: replace --width, --height by --colums, -col --rows, -row new --top, -tp --left, -lt replace --vinc, --hinc by --xoddinc, -xoi --xeveninc, -xei --yoddinc, -yoi --yeveninc, -yei As you proposed I will create a solution that works for all sensors, your idea with function function pointers in sensor_def is a good start. After Christmas I will be able to work with imx219 sensor -- I only know because my family wanted me to do the order for my christmas present to make sure I get the one I want (I ordered the NoIR v2 camera). Before doing next pull request I will test on both sensors to make sure that everything works on both sensors. I don't need my development history. If that is easier for you I can create another fork when my changes are complete based on your raspiraw branch. Then I will bring in all my changes and create a pull request from that branch. I can create another fork with just the imx219 register settings for mode 2-7 determined by measuring the I2C traffic, or provide them via raspiraw pull request. Please let me know what you prefer.

…

On 2017-12-15 14:35, 6by9 wrote: You'll need some new git magic to clean some of this up. A very brief summary: git remote -v will list your remote branches. Depending on how you forked my branch you will either have one or two entries here. To avoid having two sets of instructions here, run git remote add 6by9 https://github.com/6by9/raspiraw.git to add my repo (This may result in having it twice, but that doesn't matter). git rebase -i allows you to rewrite your local branch, reordering, dropping, or squashing commits together. It'll list out all your differences from what it considers to be upstream (if this is your own branch then it may consider them no different and you need to use git rebase -i 6by9/master to tell it where to rebase based on). The default is to pick each commit, but replace "pick" with e allows you to edit it, "s" squashes it into the previous commit, and removing the line drops the commit totally. (There are a few other options that are documented in the editor when you rebase -i) The other thing is that we don't care about your development history. These changes would ideally be atomic little additions, but TBH I'm happy it being one big hit. That is where squashing commits comes in. I've actually done that work for you and pushed it all to a new branch called "merged" (https://github.com/6by9/raspiraw/tree/merged). I'll try to review that as a whole soon, but I've already made a couple of comments on it. -- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [1], or mute the thread [2]. * Links: ------ [1] #1 (comment) [2] https://github.com/notifications/unsubscribe-auth/AMDbnmfCHq8uoGi2NdCWmGxZ9jwfBfehks5tAnWvgaJpZM4RALn7

[6by9]

Cleaned up and merged manually.

Patches really need to be self-contained and complete, unless it really is a development "hack it together" branch (in which case it shouldn't be the source of a PR).

• A master branch doesn't want the "something wrong" / "revert something wrong" type commits.
• Every commit should build (git rebase -i and/or git commit --amend to fix them, with git push -f if you've already pushed the incorrect changes)
• PRs also ought to be rebased rather than having the parent branches merged. Git does manage a lot of that, but can become ugly all too easily.
• Excess white space (trailing spaces/tabs on blank lines) should be avoided. How rigidly enforced that is depends on the project, but generally reflects a lack of attention to detail. git show even shows it up in red, so it tends to be fairly obvious if you look at your patches.

[6by9]

And I must say thanks for all your efforts here too :-)