959 add exif date time to images

[Linwood-F]

This creates an EXIF JPG header containing the DateTimeOriginal and SubsecTimeOriginal captured with the image, so that external programs using the image have a digital timestamp. This only timestamps images written with the Image class, which appears to be the "normal" path. There is at least one path through the frame server (which does not work properly anyway, at least in my testing) that this does not effect (but does not change/hurt).

The change adds 84 bytes to all images captured, and negligible overhead to writing.

[knight-of-ni]

Looks good, but we will need to create a ZM_ variable to turn this on/off under options.
I will need to look up an example of how it is done to show you.

[Linwood-F]

Is that one that appears on the options screen? I'll do that if you really prefer, but I think it is worth asking "why?". There are a lot of options, and many seem to allow control over things that are differences without significance (e.g. the associated timestamp on capture)?

Is the thinking "maybe it will break then you can turn it off"? Or is the idea that every feature added must be optional, period?

I did see the code for adding the analytical frame timing option (#956) (it required a LOT of code to get the option in), one reason I did not think this merited it. Is that an example of what you expect?

[knight-of-ni]

Heh, welcome to software development. :-)

As far as your question why:

1. It's new and experimental. We need to be able to turn it off if a problem arises.
2. It is an optional add-on feature that is not necessary for the basic operation of ZoneMinder.
3. It consumes resources, at a time when we are taking steps to make ZoneMinder use fewer resources.

I've been meaning to do something similar to another feature in ZoneMinder. When I get the time to work on that other feature and refresh my memory, I'll help step you through this.

[connortechnology]

Yeah... anytime there is a performance cost I think it needs a switch to turn it off.

[Linwood-F]

I certainly get (1), and have no objection to doing it.

I do however just offer the observation that one thing that makes ZoneMinder a bit more challenging to get going is the large number of often incompatible or at least inconsistent options. And some of those, in the vein of trying to make them optional, actually make performance worse (while minor, the timestamp-on-capture for example, copies the image an extra time even if you are not making a timestamp because your camera already does).

I wonder if there isn't a more newbie friendly way of preserving the "turn off bad stuff" while not giving people such a daunting list of options.

Anyway... to connortechnology's comment -- I can't see how this can have a measurable, much less noticeable, impact on performance. It is linear code, no loops, no compression, no extra copies of the image. It just formats a date and stuffs it in 84 bytes of data. With the average image size perhaps being a couple hundred K (?), I think the size change is in the hundredth of a percent range.

But I do understand the idea of "new, might break something, need a circuit breaker".

[knight-of-ni]

This pr puts EXIF data into every, single jpeg recorded to disk, correct?
That comes to 84 bytes of data, times the frame rate of the camera, times the number of cameras.
So while you haven't added any loops to the code, WriteJpeg is being called over and over, indefinitely. That does add up.

What you don't see are the recent effort to get zoneminder to use fewer resources. This pr runs contrary to that.

Additionally, this is a feature that few are going to use. We have to take that into account.

Yes, more options are a double edged sword, but that is more of a gui issue. In a perfect world, we would change the way we present many of the features to the end user by hiding or burying the less important widgets.

[Linwood-F]

Remembering I'm sold on the need based solely on "new and might break"....

I get the straw and camels back analogy, though I do think there is a point of diminishing returns.

One reason for putting this here publically is to see if there IS interest. Frankly if there is no one speaking up who thinks EXIF timestamps are useful, it probably should not be added to the code anyway. There's already enough ... less than useful features floating out there. I'm a huge believer that simple = reliable.

To the point of "anyone interested" maybe I should explain its context, as it may be that the reason for it is more useful as a feature than the exif itself. I did a "Real time extract" option in Zoneminder, changing the write so that a selected fraction of images could be extracted as an additional copy and written to another directory. That other directory is a queue to upload to offsite storage, as a hedge against the zoneminder server being destroyed in whatever event it may be intended to witness. That offsite storage is the relevant consumer of the exif information (namely amazon cloud drive, where timestamped images are a bit easier to handle), moreso than the zoneminder repository.

I had more or less decided that the extract itself was not worth the effort to add as a real option as I found no discussion of such needs, and thought it would be bulk without benefit. But if I was going to add an explicit user option for the EXIF, I'd probably want to at least consider just paralleling it with an option to do this real time capture (as adding 2-3 options is about as easy as adding one).

To me the current mechanism of filter and archive uploads are... at best.. too slow, and at worst unwieldy. It is aimed at infrequent (relative to capture rate) uploads of zip/tar packaged files, implying something on the far end must be prepared to unpack and utilize them, but more importantly meaning the event has to be closed, the filter period run... if you wanted to capture near real time saves in an alarm condition, a thief would be carrying out your zoneminder server before it even started uploading. :)

With cloud storage becoming almost free, and home connections faster all the time, one can easily upload a reasonable portion of the capture offsite.

So in that vein -- is the exif even worth merging at all (with an option)?

Or more broadly, would there be interest in a near-real-time extract, including the exif?

Again, I think simple = reliable, generally, so if something is not deemed useful in the core product, it probably should not go there.

[newburns]

I can vote for the need of EXIF data. Using Zoneminder in any government, police, or court surveillance, EXIF would be a plus.
However, real time EXIF is not something I even understand. When you pull down the event's images, you will have EXIF data embedded, so when is "Real Time" EXIF used and how?

[Linwood-F]

My use of "real time" was in the context of extract, not exif. Two different things, though the extract in my case made exif desirable. I was using the term "real time" to differentiated between what I wanted for extract, and the deferred, batched extract now available.

I am using the timestamp that the image carries with it inside of the event object. I have not tracked back through it far enough to see exactly where that comes from. I assumed (dangerous of course) it was tagged on capture and propagated, but a recent posting about pre-alarm buffers makes me wonder. But it is the same timestamp that is applied as text to images if that option is selected.

[knight-of-ni]

Yes, I think it is worth merging as long as it is optional. You aren't the first person to request an optional feature to ZoneMinder, and you won't be the last.

What does make you unique personally, and rather valuable, is your willingness to actually do the bulk of the work. We get all sorts of requests like analytics, camera based motion detection, onvif, you name it, but the OP will usually baulk when it comes time to write the code. When it is just a few people working on a project in their spare time, it takes more than a "hey, I've got this neat idea".

What I'm getting at is, as long as you are willing and able to create the pull requests, then we are generally willing to incorporate it.

Yes, this can ultimately lead to information overload, but ZoneMinder already suffers from that. I think we should treat that as a separate problem in itself. Just because we create a new feature, does not mean that we have to present it to the end user in every case (variables can be "hidden"). For example, simply writing a new skin, with just a minimal set of features, might be enough to make an easier experience for the basic user.

[Linwood-F]

So, @knnniggett , did you mean "it" as in the exif, or "it" as in both that and the real time image extract?

Actually the only reason I'm using ZoneMinder is so I can write code. I did a pretty wide review of software and it was not really at the top otherwise. But it's real attractive to have a product which you can just fix if you don't like something!

[newburns]

@knnniggett
Don't be down talking my neat ideas!
I put a lot of work into thinking up those neat ideas. I probably wouldn't be using some of the features that I requested, but those ideas were still great. And I did more than a "Hey". I actually put your name in some of them.

Just so it is understood, I am one of the people that follow Zoneminder closely that cannot code at all. I can attest to the random great idea, and speaking from someone who has had ideas submitted here, I can say with great gratitude "Welcome aboard @Linwood-F "
I was not at all serious about my downplay on neat ideas, and I do see both sides of the coin. I thank the Zoneminder developers we have, and their attention to detail. Trying to get things right even when there is no real bug reported. Going over every small detail on features like this, until you get an end product that is ridiculously amazing.

[connortechnology]

I actually really like the idea of having the exif data filled in. I don't think I will ever use it, but I like it.

I also think the idea of multiple storage paths is good too. It is something that I am working on too.

And I want to thank you for providing code. We will get this in there.

[knight-of-ni]

@Linwood-F I meant this entire pull request is worth merging with the modifications previously mentioned.

@newburns I didn't have anyone in particular in mind when I wrote that. Note, however, that I am actively closing issues that don't fit the category of a bug, or the issue is a feature request with no associated bounty or associated pull request. Our github forum is not intended to discuss things for the sake of discussion. If you have an idea but aren't sure how to proceed, I would encourage you to discuss it first in our irc channel or the user forum. The same is true for questions there are not bug related or are general use questions.

This isn't personal, and it certainly does not mean I don't like the idea being presented. I've had many of the same ideas myself. Moving forward, I am taking a harder stance on this because the number of issues in our github forum is getting out of hand. Many of the issues are not appropriate, or will never be worked on, because the author is not willing to write the code or provide a bounty. Having these kinds of issues mixed in makes it harder for us to discover the real issues that actually can be solved.

[Linwood-F]

OK, I'll put together the extract with the exif. In some ways that is easier as I was trying to keep them separate (git vs my copy) which kept me continually creating unwanted files to upload.

Is the analysis timeframe skip a good sample of adding an option? Or were you suggesting a less overt on/off switch, like at build time?

[newburns]

LOL. @knnniggett
It's quite apparent you didn't read my post entirely. To sum it up, I am 100% with you, and I congratulate all of the developers here on their optimum coding skills and attention to detail.

I promise not to clog this up anymore. I understand the need to keep Github clean. Thanks again for all the great work

[knight-of-ni]

@newburns Actually, I read it several times, which is why it took so long for a reply. Thank you for your support. I am simply trying to get you to follow the proper channels moving forward. Help us to help you.

[knight-of-ni]

@Linwood-F just sent you a PR which creates a toggle to turn this feature on/off.

[Linwood-F]

I've merged this into my repository and then fixed to turn off and on. I've tried it defaulted, checked, and unchecked and it was OK, and the database update script ran OK. I checked the images outside of zoneminder and they had (or not) exif as expected. I see it says there are conflicts, but I'm not knowledgeable enough about git to figure out where they are. I'll jump on the irc in the morning and see if you or someone is around to help.

Postscript: OK, maybe I figured it out, I had to bring the specific branch up to date on my end not (just) master. Maybe. I don't know what I'm doing so take a good luck to see like it got updated correctly.

[knight-of-ni]

Can you please tell us what you did to bring your branch up to date?

I'm not sure I know how to programmatically verify this. Maybe @connortechnology does?

99c8af4 has got to be 100% perfect, otherwise some else's commit(s) will get removed when we merge your pr.

[Linwood-F]

Well, maybe.

I added an upstream link: git remote add upstream https://github.com/ZoneMinder/ZoneMinder.git

I fetched upstream

checkout master

git merge upstream/master

Resolved those issues and committed and pushed.

checkout 959-add-exif-date-time-to-images

This is where the memory gets vague -- I tried a few variations of the command to merge into that branch. i THINK the one that worked was merge upstream/959-add-exif-date-time-to-images but not completely sure, but conceptually I did the merge both into my master and into my 959-add-exif-date-time-to-images branch, resolved any discrepancies in each, committed and pushed.

I built and tested with the result, plus reviewed the changes as it shows up on git in the pr, which looks right for my stuff and matches as best I can visually compare with your PR on my fork.

But I am not at all sure the principle I had was correct -- to merge both master and 959-add-exif-date-time-to-images branches on my system, but the former had to be done before it would say ready to merge on the PR.

Did I make a mess somehow?