search

Warzone2100 / old-trac-import

Archived Import of (old) Warzone 2100 Trac
0 stars 0 forks source link

Adding back JPG support for screen shots #23

Closed wzdev-ci closed 14 years ago

wzdev-ci commented 16 years ago

resolution_fixed type_patch (an actual patch, not a request for one) | by Buginator

I added back JPG support for screenshots.

It will be a configurable option in the config file. The options will be 1 for PNG, 2 for JPG, and 3 for BOTH.

The mac build system will need to be updated. The linux build system should work, as I tested on debian. The windows (MSVC) build system works.

If you have any objections, speak up!

Issue migrated from trac:23 at 2022-04-15 17:38:25 -0700

wzdev-ci commented 16 years ago

Buginator uploaded file JPGnPNG4.patch (20.4 KiB)

wzdev-ci commented 16 years ago

Giel changed milestone from ` to2.2`

wzdev-ci commented 16 years ago

Giel commented

Please update lib/ivis_common/makefile.win32 and warzone2100.cbp as well.

And after you commit it, please notify the ML that the Xcode project will require an update. Also give me a shout when you've committed as I'll need to update the libraries of my nightly build system then. (I'll CC myself to this ticket to monitor it, so just adding a notification here will do).

Also could you mark your #include from src with something like:

#!c
// FIXME: Include from "src"

As that will require an architectural change.

And as a last annoyance ;-) , please use Subversion's copy functionality to preserve history. If you need help with that, just ask (although my TortoiseSVN experience isn't up-to-date, I'm more proficient with command line utilities).

wzdev-ci commented 16 years ago

Baum _uploaded file xcode_jpg_util.diff (4.1 KiB)_

Adds jpg_util.h and jpg_util.c to the Xcode project file

wzdev-ci commented 16 years ago

anonymous commented

Ok, thanks Baum for the xcode update.

The plan is to commit this patch, this weekend.

wzdev-ci commented 16 years ago

Giel uploaded file win32-makefile-and-codeblocks-project.patch (1.1 KiB)

Updates win32 Makefiles and Code::Blocks project files

wzdev-ci commented 16 years ago

Giel commented

Replying to Warzone2100/old-trac-import#23 (comment:4):

The plan is to commit this patch, this weekend. Uh oh, that'll break the nightly builds. And I'll be gone to London until Thursday.

Just go ahead and commit it though, I'll fix it when I get back. People will just have to do without nightlies for less than a week then.

That aside, some further remarks about your patch:

I appreciate that you're so eager to add API documentation to some functions. But currently you're not using Doxygen documentation blocks (we mostly use the JavaDoc style), so Doxygen won't extract them. You might also want to take a look at Doxygen's special command list.

Furthermore, a lot of your comments are describing how the code works. Please don't describe that in comments, as that should be obvious from the code itself, if it's not then your code probably needs improvement. E.g. at one point you wrote:

"in this routine, we allocate enough memory for the readback of the framebuffer and then depending on the user's choice, we save the framebuffer as either a png (1) or jpg(2) or both(3) file formats. Then we free the buffer we used."

What you're doing here is describing the algorithm that's documented by the code itself. I.e. "allocate memory" and "Then we free the buffer we used.". Surely I can read the code myself? Also the last part (about freeing the buffer) isn't correct, as we only free the buffer when the screen size changes (which is currently never). Secondly, from an API perspective (as I assume you intended that comment as API documentation), it's not interesting that there's memory being allocated in that function if that memory is never exposed to the caller.

See [5821] for an example of a syntactically valid Doxygen documentation block. It also doesn't concern itself with implementation details that aren't interesting to callers of that function.

PS The article Coding Without Comments probably explains the commenting thingy better than I do.

Then in screenDumpToDisk you just copy pasted the while loop to search for spare filenames. I recommend you move it to a function instead. Also because you set

#!c
screendump_num = 0

you introduce a bug. What if the JPEG number needed to be higher than the PNG number? As a result you're now writing over an already existing JPEG file. It's probably better not to set it to zero.

Then the last thing I noticed, you changed the "Saving screenshot " debug output. Writing out "and/or" is probably a lot better there. It took me a while (at least 1 minute) to figure out what it meant.

Then lastly to bug you about the SVN copy thing again, would you please copy jpg_util.c from source:trunk/lib/ivis_opengl/screen.c@1489. Using the command line client you can just enter this command (in the root of your working copy obviously):

svn copy -[1489] svn+ssh://<username>@svn.gna.org/svn/warzone/trunk/lib/ivis_opengl/screen.c lib/ivis_common/jpg_util.c

With TortoiseSVN you can take these steps:

I realize this "comment" has become more of a blog posting by now, my apologies for this if the sheer size offends you.

wzdev-ci commented 16 years ago

Giel uploaded file JPGnPNG5.patch.zip (43.2 KiB)

Updated patch (resolves merge conflicts and includes build system patches). This patch requires the "svn copy" operation mentioned in comment:5. (zipped to circumvent the upload size limit)

wzdev-ci commented 16 years ago

anonymous commented

O_o

Anyway, I guess I will wait, since I don't think having the ML spammed with the build bot is broken messages will do us any good.

For the commenting, while the code should speak for itself, I don't think it hurts as much as the link (or you) suggest. While it may be obvious today, and tomorrow, that doesn't mean it will always be obvious sometime in the future, the intent of what the coder wanted.

For the freeing of memory, (when we are done dumping the screen to disk), I do a free(image.bmp); That does free the image buffer we allocated. I guess I should have been a bit more clear.

For the resetting of screendump_num to 0, I only did that since most of the time, if you have it set to dump both formats, if one is named blah192.png, you expect the other to be named blah192.jpg, not blah266.jpg (or whatever).

For the &/| ... heh :o

I'll make a mental note about doxygen, but you overloaded my timeframe with your novel. ;)

For the svn copy... blasted GNA. I have been getting the 'connection closed unexpectedly' error way too much to do anything like that right now. Hopefully, by the time you get back, it will work better.

wzdev-ci commented 16 years ago

Buginator commented

erm, that was me, dunno why I keep getting logged out. Perhaps there is a time limit or something?

wzdev-ci commented 16 years ago

Giel uploaded file JPGnPNG6.patch (45.0 KiB)

Updated patch

wzdev-ci commented 16 years ago

Giel commented

@DevUrandom: I'm having trouble cross compiling libjpeg. Could you lend me a hand in this regards?

The problem is that the available configure script apparently ignores the --host cross compilation parameter.

wzdev-ci commented 16 years ago

anonymous commented

I am trying not to laugh... I said for a reason that I was happy to remove libjpeg from the list of dependencies. ;)

And currently I really do not have time, remind me again in 2 weeks soonest.

Question on a different topic: Did we leave Gna now and do all bugtracking here? Are all bugs already converted/moved-over?

wzdev-ci commented 16 years ago

Giel commented

Replying to Warzone2100/old-trac-import#23 (comment:9):

I said for a reason that I was happy to remove libjpeg from the list of dependencies. ;)

Believe me, I don't really like jpeg as a dependency either. But I like broken windows builds and nightly builds even less.

Question on a different topic: Did we leave Gna now and do all bugtracking here? Are all bugs already converted/moved-over?

No to both. I've been looking into converting an exported bug & patch database into something Trac can handle. But I haven't got all that much time either. One problem would be that all bug & patch numbers need to be retained, and that Trac should be able to understand the "bug #nnn" and "patch #nnn" syntax. Although that might just as well be the easy part... Nah, converting the XML into a series of SQL statements is probably more difficult.

wzdev-ci commented 15 years ago

Zarel changed blocking which not transferred by tractive

wzdev-ci commented 15 years ago

Zarel changed blockedby which not transferred by tractive

wzdev-ci commented 15 years ago

Zarel commented

Status?

wzdev-ci commented 15 years ago

Buginator commented

While it works, the problem is with cross-compiling, so this still is on a wait cycle.

wzdev-ci commented 15 years ago

Buginator changed milestone from 2.2 to 2.3

wzdev-ci commented 15 years ago

Buginator commented

bumping to 2.3 I guess.

wzdev-ci commented 14 years ago

cybersphinx uploaded file 0001-add-jpeg-support.patch (44.6 KiB)

JPEG screenshots with included encoder

wzdev-ci commented 14 years ago

cybersphinx commented

I made a little patch that doesn't add new dependencies, but includes a small jpeg encoder (from http://blog.frankvh.com/2009/06/09/blackfin-fast-jpeg-encoding/, slightly changed to not use the assemby routines).

The patch is a hacked together proof of concept right now, with the jpeg save routine added to png_util.c, no test if the jpg file exists (I just use the png filename and change the extension to .jpg), no configuration...

Comments, testing etc. welcome, if it is deemed ok I can clean it up into a state fit for committing.

wzdev-ci commented 14 years ago

cybersphinx uploaded file add-jpeg-support.patch (44.8 KiB)

Screwed up TortoiseSVN version.

wzdev-ci commented 14 years ago

Buginator commented

cybersphinx, it still contains gcc specific code, all the attribute stuff is used to force that code / variable to be stored in the CPU cache...

Not sure what kind of a performance hit you would take if we removed all the attribure stuff?

wzdev-ci commented 14 years ago

Buginator commented

Here is the fixed version.

wzdev-ci commented 14 years ago

Buginator uploaded file jpg-patch-non-attributes.patch (44.6 KiB)

wzdev-ci commented 14 years ago

Giel commented

Replying to Warzone2100/old-trac-import#23 (comment:15):

cybersphinx, it still contains gcc specific code, all the attribute stuff is used to force that code / variable to be stored in the CPU cache...

Not sure what kind of a performance hit you would take if we removed all the attribure stuff?

As a result of ditching the l1_data attributes no performance will be lossed as it's specific to the Blackfin series of DSP processors. Removing l1_text might in fact cause a speedup as l1_text forces the code into L1 cache (overriding the compiler's optimizer and the CPU's optimisation hardware).

As for the alignment, I'm reasonably sure that most compilers on 32bit systems will align that data on a 4 byte boundary anyway.

Either way, this code looks like its painfully hand optimized code for a very specific hardware platform. It looks very had to debug/maintain as well. In fact those tables look like they're automatically generated to me (in fact I'm sure of thta), I'd rather have the code (or set of equations plus parameters) that generates those tables than those tables...

wzdev-ci commented 14 years ago

cybersphinx commented

Well, if you can find some better code, I don't mind using that. This one is nice since it is only two files/one function call. Basically the JPEG code is a blackbox, we don't need to do any maintenance on it (as long as it works, that's why I made this patch to test it).

wzdev-ci commented 14 years ago

Buginator commented

I still think this should be stuck in trunk...

wzdev-ci commented 14 years ago

cybersphinx uploaded file 0001-Save-screenshots-as-JPG-and-PNG.patch (37.6 KiB)

Updated patch with some cleanup.

wzdev-ci commented 14 years ago

_thecybersphinx commented

(In [11083]) Save screenshots as JPG and PNG.

lib/ivis_common/jpeg_encoder.c/h is the JPEG encoder from http://blog.frankvh.com/2009/06/09/blackfin-fast-jpeg-encoding/ (GPL v2+), cleaned up to be C only. Refs #23.

wzdev-ci commented 14 years ago

_thecybersphinx commented

(In [11086]) Save screenshots as JPG and PNG.

lib/ivis_common/jpeg_encoder.c/h is the JPEG encoder from http://blog.frankvh.com/2009/06/09/blackfin-fast-jpeg-encoding/ (GPL v2+), cleaned up to be C only. Refs #23.

wzdev-ci commented 14 years ago

Git SVN Gateway gateway@... commented

(In [03a75906cdbf5e268c420e11162ca43bd9000fd5]) Save screenshots as JPG and PNG.

lib/ivis_common/jpeg_encoder.c/h is the JPEG encoder from http://blog.frankvh.com/2009/06/09/blackfin-fast-jpeg-encoding/ (GPL v2+), cleaned up to be C only. Refs #23.

git-svn-id: https://warzone2100.svn.sourceforge.net/svnroot/warzone2100/trunk@11086 4a71c877-e1ca-e34f-864e-861f7616d084

wzdev-ci commented 14 years ago

_thecybersphinx changed status from new to closed

wzdev-ci commented 14 years ago

_thecybersphinx set resolution to fixed

wzdev-ci commented 14 years ago

_thecybersphinx commented

(In [11131]) 2.3: Save screenshots as JPG and PNG.

lib/ivis_common/jpeg_encoder.c/h is the JPEG encoder from http://blog.frankvh.com/2009/06/09/blackfin-fast-jpeg-encoding/ (GPL v2+), cleaned up to be C only. Closes #23.

wzdev-ci commented 14 years ago

Git SVN Gateway gateway@... commented

In [03a75906cdbf5e268c420e11162ca43bd9000fd5]:

#CommitTicketReference repository="" revision="03a75906cdbf5e268c420e11162ca43bd9000fd5"
Save screenshots as JPG and PNG.

lib/ivis_common/jpeg_encoder.c/h is the JPEG encoder from
http://blog.frankvh.com/2009/06/09/blackfin-fast-jpeg-encoding/ (GPL v2+),
cleaned up to be C only. Refs #23.

git-svn-id: https://warzone2100.svn.sourceforge.net/svnroot/warzone2100/trunk@11086 4a71c877-e1ca-e34f-864e-861f7616d084
wzdev-ci commented 14 years ago

Git SVN Gateway gateway@... commented

In [03a75906cdbf5e268c420e11162ca43bd9000fd5]:

#CommitTicketReference repository="" revision="03a75906cdbf5e268c420e11162ca43bd9000fd5"
Save screenshots as JPG and PNG.

lib/ivis_common/jpeg_encoder.c/h is the JPEG encoder from
http://blog.frankvh.com/2009/06/09/blackfin-fast-jpeg-encoding/ (GPL v2+),
cleaned up to be C only. Refs #23.

git-svn-id: https://warzone2100.svn.sourceforge.net/svnroot/warzone2100/trunk@11086 4a71c877-e1ca-e34f-864e-861f7616d084
wzdev-ci commented 12 years ago

cybersphinx changed milestone from 3.0 to unspecified

wzdev-ci commented 12 years ago

cybersphinx commented

Milestone 3.0 deleted