Opened 5 years ago

Last modified 16 months ago

#2828 new defect

Decide JPEG support

Reported by: historic_bruno Owned by: Itms
Priority: Nice to Have Milestone: Work In Progress
Component: Core engine Keywords: patch
Cc: Patch:

Description (last modified by elexis)

Currently the game requires libjpeg as a dependency, even though we don't use it for anything. Game art should never use JPEG, but a lossless format like PNG. Really the only thing I could imagine us wanting JPEG for is screenshots, but even on high quality settings it will lose a lot of detail.

On Windows, we use a very old version of libjpeg, but we should decide if it's worth supporting JPEG at all, and if not, just remove it from the game.

Attachments (3)

t2828_v1.patch (4.0 KB) - added by Sergey Kushnirenko 4 years ago.
t2828_v1.zip (893.7 KB) - added by Sergey Kushnirenko 4 years ago.
jpeg-cleanup.patch (2.0 KB) - added by Itms 3 years ago.
first draft that Trac didn't let me upload yesterday

Download all attachments as: .zip

Change History (29)

comment:1 Changed 5 years ago by Niek

I say remove it. Screenshots use PNG, big screenshots BMP. I can't recall any occasion where jpeg is used.

comment:2 Changed 5 years ago by stanislas69

I'm also for removing it. The only case where it could be useful is when making a big screenshot, cause on windows it cause OOM's. So it should try to make it bmp and if it can't make it jpeg. But that's a hack.

comment:3 Changed 5 years ago by yashi

FYI

Debian and Ubuntu version of libnvtt is linked against a version of libjpeg.

comment:4 Changed 5 years ago by ben

In 16198:

Updates prebuilt libjpeg win32 binaries using VC++ 2013 (v120_xp toolset), refs #2828

comment:5 Changed 4 years ago by fabio

In 17621:

Update all OS X libraries to their latest versions (exlcuding currently unused libjpeg, see #2828). Some also include security fixes. Fixes #3721.
Also disable unused wxwidgets features. Patch by trompetin17, refs #2924.
Tested by wraitii.

comment:6 in reply to:  2 ; Changed 4 years ago by historic_bruno

I've had a change of mind about this. We don't use jpeg and nobody plans to, so I favor removing it.

Replying to stanislas69:

I'm also for removing it. The only case where it could be useful is when making a big screenshot, cause on windows it cause OOM's. So it should try to make it bmp and if it can't make it jpeg. But that's a hack.

It probably wouldn't help with process memory, in fact it could make things worse, the uncompressed image still has to be there before compressing it with jpeg. The only gain would be smaller files on disk, but disks are cheap and huge these days.

comment:7 in reply to:  6 Changed 4 years ago by leper

Keywords: simple added

Replying to historic_bruno:

I've had a change of mind about this. We don't use jpeg and nobody plans to, so I favor removing it.

Agreed, there's no reason to keep it.

Changed 4 years ago by Sergey Kushnirenko

Attachment: t2828_v1.patch added

comment:8 Changed 4 years ago by Sergey Kushnirenko

Keywords: review patch added
Milestone: BacklogAlpha 20
Summary: Decide JPEG support[PATCH] Decide JPEG support

Changed 4 years ago by Sergey Kushnirenko

Attachment: t2828_v1.zip added

comment:10 Changed 4 years ago by historic_bruno

Thanks for the patch!

I think the change to NVTT isn't necessary. Even if it's built with JPEG support, we don't use it (and that should be configured with CMake, anyway).

The file source\lib\external_libraries\libjpeg.h and the reference to libjpeg in libraries\README.txt can be removed.

comment:11 Changed 4 years ago by Itms

Owner: set to Itms
Resolution: fixed
Status: newclosed

In 17870:

Remove libjpeg. Fixes #2828.

Based on patch by dalerank, tested on Windows, might need to be amended for OSX.

comment:12 Changed 4 years ago by Itms

Keywords: simple review removed

I removed libjpeg and its use in wxWidgets but didn't touch NVTT, which didn't use jpeg at all (-DJPEG=0)

There was also an extra space in tex_codec.cpp in your patch, which I removed.

Thanks for taking care of that!

comment:13 in reply to:  description Changed 3 years ago by elexis

Resolution: fixed
Status: closedreopened

Replying to historic_bruno:

even though we don't use it for anything

Replying to niektb:

I can't recall any occasion where jpeg is used.

Replying to leper:

there's no reason to keep it

Replying to Itms:

removed libjpeg

  • It was nice to be able to write screenshots in JPEG format. F.e. we could add an option whether to take screenshots in JPG or PNG format. As Stan said, this might be useful for big screenshots in particular. People might take many screenshots and thus decide to use the format with the significantly lower filesize. I wonder whether that shouldn't become default even, since most screenshots aren't used for promotional material.
  • The feature was being used for atlas heightmap import.

Now we get a crash when trying to open a "Valid Image file" as an atlas heightmap, (and a crash with a meaningless message when trying to take a screenshot in JPEG format, which was possible before).

Indeed we don't use JPEG textures anywhere and likely never will. Please decide to either revert or fix those occurances of "jpg" and "jpeg" in source/ which one thinks would have been checked before the removal.

I guess we don't use a library for BMP support, so nothing to remove there.

(My opinion: I'll revert that commit locally to support JPEG screenshots).

Last edited 3 years ago by elexis (previous) (diff)

comment:14 Changed 3 years ago by Itms

Milestone: Alpha 20Backlog
Status: reopenednew
Summary: [PATCH] Decide JPEG supportDecide JPEG support

comment:15 Changed 3 years ago by Itms

Milestone: BacklogWork In Progress

Here is the beginning of something but

  • There is still a reference to jpg in the archive builder. I would remove it but there appears to be three jpg files in our textures. We should make sure they are not used at all (probably not since we removed libjpeg without issue).
  • Since we only support bmp for big screenshots and png for small ones, I suppose we should rewrite the WriteScreenshot function to take a bool big argument instead of an extension one. Unless people want a BMP small screenshot, but a boolean might be neater in any case.

This doesn't warrant an actual review yet, so I'm making use of our new milestone :o)

comment:16 Changed 3 years ago by fabio

I don't see the need to have JPEG support for screenshots. Available BMP and PNG formats are lossless, if someone really want a smaller file, with reduced quality, can convert the screenshot to JPEG later.

Changed 3 years ago by Itms

Attachment: jpeg-cleanup.patch added

first draft that Trac didn't let me upload yesterday

comment:17 Changed 3 years ago by elexis

The main reason for reopening this ticket was the occurance in ScenarioEditor.cpp listing jpg files as a valid image format in the heightmap import tool, ending with that meaningless crash message.

comment:18 Changed 3 years ago by elexis

( For those interested in JPEG screenshots:

Use svn merge -c -17870 to restore jpeg support and change png to jpg in main.cpp.

The screenshot size is 200kb on average and finished in a fraction of a second, while a png screenshot freezes 0 A.D. for 2 seconds here and consumes about 10x the size without having any noticeable difference to the human eye, unless one is zooming in or doing promotional material. )

Last edited 3 years ago by elexis (previous) (diff)

comment:19 Changed 3 years ago by wraitii

Regarding the "freeze for 3 minutes". That's probably because we're saving the file synchronously, which we really should not be doing.

comment:20 in reply to:  17 Changed 3 years ago by Itms

Replying to elexis:

The main reason for reopening this ticket was the occurance in ScenarioEditor.cpp listing jpg files as a valid image format in the heightmap import tool, ending with that meaningless crash message.

Oh right, I had forgotten Atlas so it didn't show up in my IDE. Thanks.

comment:21 in reply to:  16 Changed 3 years ago by elexis

Replying to wraitii:

"freeze for 3 minutes"

Didn't say minutes, but it's indeed less than 3 seconds (anyway a brief noticeable freeze here when saving PNG)

Some thoughts about the potential of JPEG screenshots:

Whenever uploading replays, the uploading of screenshots of the relevant parts of the replay helps the reader and replayer identifying it's contents / relevance. For example when reporting bugs (PNG ones often exceed the 2mb upload limitation), but more importantly the a21 replay thread. Saving JPEG files directly saves disk space and the additional conversion work, thus making it easier for players to upload screenshots (certainly incentivized me to upload more replays).

A third use case might be a new optional feature: Taking a picture of the map or minimap and showing that in the replay / savegame dialog, so that the games are easily distinguishable (even if they share the same gamesettings and players). Saving them in JPEG would be kind of mandatory regarding the disk space (a jpeg image < 200kb is not that significant in comparison to a 1-2MB replay file).

comment:22 in reply to:  18 Changed 3 years ago by Vladislav Belov

Replying to elexis:

The screenshot size is 200kb on average and finished in a fraction of a second, while a png screenshot freezes 0 A.D. for 2 seconds here and consumes about 10x the size without having any noticeable difference to the human eye, unless one is zooming in or doing promotional material.

I see the only one way to leave jpeg for screenshots: allow to user decide which format should be used for screenshots, i.e. in options, as you suggested above.

About heightmaps with JPEG: I think artifacts of JPEG could be noticeable on huge maps.

Last edited 3 years ago by Vladislav Belov (previous) (diff)

comment:23 Changed 2 years ago by elexis

Description: modified (diff)

To use JPEG screenshots in alpha 22:

svn merge -c -17870 .

Replace rpU8 with u8* RESTRICT in tex_jpg.cpp following r19184. Replace png with jpgin main.cpp.

Patch to provide JPEG and PNG config option at Phab:D779.

Last edited 2 years ago by elexis (previous) (diff)

comment:24 Changed 20 months ago by elexis

In 21112:

Stop claiming that JPEG files are supported for heightmap import following rP17870, refs #2828.

comment:25 Changed 20 months ago by elexis

I couldn't load any BMP file either, but maybe I encoded them incorrectly. TIFF and DDS ought to be supported too, nay? Couldn't get them to load either. I didn't remove the comments, because JPEG support should become optional at least (Phab:D779).

comment:26 Changed 16 months ago by elexis

To use JPEG screenshots in alpha 23: do the same as in comment:23, but pass --premake4 to update-workspaces.sh and rename pageSize to g_PageSize following r21480. See also implementation in https://github.com/elexis1/0ad/commits/trailerhacks_a23

Note: See TracTickets for help on using tickets.