This repository has been archived by the owner. It is now read-only.

Add JPEG-XR support #105

Closed
wolfbeast opened this Issue Jun 17, 2015 · 45 comments

Comments

Projects
None yet
4 participants
@wolfbeast
Member

wolfbeast commented Jun 17, 2015

As an alternative and standardized format for images, JPEG-XR (extended range jpeg, especially suitable for HD photography, wide-gamut images, and a superior compression format compared to standard JPEG) would be good to have. This format was rejected by Mozilla based on mostly political reasoning. Adding this alongside WebP should provide maximum choice for Pale Moon users.
A basic implementation with decode-after-load should be straightforward.

On hold for the moment as I communicate with the original coder and/or MS OpenTech to get a suitable implementation for our code base.

@wolfbeast wolfbeast added this to the 26.0 milestone Jun 23, 2015

@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Jul 27, 2015

Member

Clearing the milestone for now since after initial communication, both the dev and MSOT have become completely unresponsive. 👎
Do they want their format to be adopted, or not...?

Member

wolfbeast commented Jul 27, 2015

Clearing the milestone for now since after initial communication, both the dev and MSOT have become completely unresponsive. 👎
Do they want their format to be adopted, or not...?

@wolfbeast wolfbeast removed this from the 26.0 milestone Jul 27, 2015

@wolfbeast wolfbeast added this to the 26.0 milestone Aug 3, 2015

@wolfbeast wolfbeast removed the On Hold label Aug 3, 2015

@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Aug 3, 2015

Member

I got a reply from MSOT, and a repo was committed for the decoder as-written for them, and released to the public that way, building on jxrlib and published under the Community Promise.
This means we can go forward. I'm keeping myself assigned to this since adding decoders is still fresh in memory.

Member

wolfbeast commented Aug 3, 2015

I got a reply from MSOT, and a repo was committed for the decoder as-written for them, and released to the public that way, building on jxrlib and published under the Community Promise.
This means we can go forward. I'm keeping myself assigned to this since adding decoders is still fresh in memory.

@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Aug 11, 2015

Member

Well, it looks like the code dump supplied by MSOT is mostly useless. It's just a dump of files as-used at an unknown code point. No diffs, no history, missing files, etc.

The build system files are incomplete, e.g. there are NO symbol&header exports listed, and trying to add them manually, both by adopting the library properly in /media/libjxr and as-supplied in /image fails with unresolved externals. I've wasted 2 days on this and can't get it to work, so maybe someone with the appropriate knowledge can look at this.

Repo of jxr FF code:
https://github.com/wolfbeast/ff-jpegxr
Integration commit (separate branch): Commit: 307b09b

Member

wolfbeast commented Aug 11, 2015

Well, it looks like the code dump supplied by MSOT is mostly useless. It's just a dump of files as-used at an unknown code point. No diffs, no history, missing files, etc.

The build system files are incomplete, e.g. there are NO symbol&header exports listed, and trying to add them manually, both by adopting the library properly in /media/libjxr and as-supplied in /image fails with unresolved externals. I've wasted 2 days on this and can't get it to work, so maybe someone with the appropriate knowledge can look at this.

Repo of jxr FF code:
https://github.com/wolfbeast/ff-jpegxr
Integration commit (separate branch): Commit: 307b09b

@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Oct 22, 2015

Member

Clearing milestone - doesn't look like the existing files are of any help (including the VSO file dump I received end of Aug). Looks like it will not make it into 26.0

Member

wolfbeast commented Oct 22, 2015

Clearing milestone - doesn't look like the existing files are of any help (including the VSO file dump I received end of Aug). Looks like it will not make it into 26.0

@wolfbeast wolfbeast removed this from the 26.0 milestone Oct 22, 2015

@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Oct 11, 2016

Member

If we want this, it will have to be a completely new implementation for v27+
Marking eligible for bounty.

Member

wolfbeast commented Oct 11, 2016

If we want this, it will have to be a completely new implementation for v27+
Marking eligible for bounty.

@wolfbeast wolfbeast added On Hold Bounty and removed On Hold labels Oct 11, 2016

@rhinoduck

This comment has been minimized.

Show comment
Hide comment
@rhinoduck

rhinoduck Oct 19, 2016

Contributor

I couldn't sleep last night so I took a look at this. Putting the code together was messy business, but once I actually got it to compile, I was met with a pleasant surprise of an image loading fine on the first run.

This is still just a preview; it is the first working version on which I did close to zero testing. I still want to (and need to) look at how and why things actually work, if they will work well in all (or most important) cases, if something can be improved... and I need to clean up a bit (including some of those rants I put into comments out of frustration; some of which may soon in turn ironically make me look like an idiot in hindsight). I'm placing this here so that you know that something is happening and that there are actual results. There's still work ahead though.

In the following branch, you can find what's been done so far; hopefully, I'll be able to continue with what I outlined above in the following days. Also provided, is a test page with one small and one large jxr image.

Preview branch: https://github.com/rhinoduck/Pale-Moon/tree/jxr-test
Test page: http://rhinoduck.net/palemoon/github/issues/105/dogontrain.html

The images were converted using the following EncApp.exe example command (don't try to judge quality though, the originals came from plain JPEG files):

EncApp.exe -i input.bmp -o output.jxr -q 10

Misc Info:

  • Pale Moon code was based on the master branch (Tycho)
  • jxrlib code was taken from wolfbeast's repo linked above in this issue
  • integration was done from scratch (all things considered, personally, I perceived it as easier than trying to merge a year old effort that I didn't know with a living branch)
  • Mozilla Build 1.10 was used (I don't know what you use with Tycho; this worked for me after fixing a small build issue with the installer/uninstaller scripts)
  • mozconfig settings were taken from the buildconfig.html file of 27.0.0b2

Here they are for easy access:

mk_add_options MOZ_CO_PROJECT=browser
WIN32_REDIST_DIR=$VCINSTALLDIR/redist/x86

ac_add_options --with-distribution-id="MCP-beta"
ac_add_options --with-branding="browser/branding/unstable"
ac_add_options --enable-application="browser"
ac_add_options --enable-optimize="-O2 -GS- -Qfast_transcendentals -Qpar -Qpar-report:1"
ac_add_options --enable-jemalloc
ac_add_options --enable-shared-js
ac_add_options --disable-debug
ac_add_options --disable-tests 
ac_add_options --disable-mochitests
ac_add_options --enable-strip
ac_add_options --disable-updater
ac_add_options --disable-crashreporter
ac_add_options --disable-accessibility
ac_add_options --disable-parental-controls
ac_add_options --disable-maintenance-service
ac_add_options --disable-sandbox

I suppose I could also provide a binary if you wanted one, though I would probably have to recompile with branding turned off.

I hope I'll have better luck with dropping dead tonight and enjoying quite a few hours of sleep, so, if lucky, I will not be responding during that time.

Contributor

rhinoduck commented Oct 19, 2016

I couldn't sleep last night so I took a look at this. Putting the code together was messy business, but once I actually got it to compile, I was met with a pleasant surprise of an image loading fine on the first run.

This is still just a preview; it is the first working version on which I did close to zero testing. I still want to (and need to) look at how and why things actually work, if they will work well in all (or most important) cases, if something can be improved... and I need to clean up a bit (including some of those rants I put into comments out of frustration; some of which may soon in turn ironically make me look like an idiot in hindsight). I'm placing this here so that you know that something is happening and that there are actual results. There's still work ahead though.

In the following branch, you can find what's been done so far; hopefully, I'll be able to continue with what I outlined above in the following days. Also provided, is a test page with one small and one large jxr image.

Preview branch: https://github.com/rhinoduck/Pale-Moon/tree/jxr-test
Test page: http://rhinoduck.net/palemoon/github/issues/105/dogontrain.html

The images were converted using the following EncApp.exe example command (don't try to judge quality though, the originals came from plain JPEG files):

EncApp.exe -i input.bmp -o output.jxr -q 10

Misc Info:

  • Pale Moon code was based on the master branch (Tycho)
  • jxrlib code was taken from wolfbeast's repo linked above in this issue
  • integration was done from scratch (all things considered, personally, I perceived it as easier than trying to merge a year old effort that I didn't know with a living branch)
  • Mozilla Build 1.10 was used (I don't know what you use with Tycho; this worked for me after fixing a small build issue with the installer/uninstaller scripts)
  • mozconfig settings were taken from the buildconfig.html file of 27.0.0b2

Here they are for easy access:

mk_add_options MOZ_CO_PROJECT=browser
WIN32_REDIST_DIR=$VCINSTALLDIR/redist/x86

ac_add_options --with-distribution-id="MCP-beta"
ac_add_options --with-branding="browser/branding/unstable"
ac_add_options --enable-application="browser"
ac_add_options --enable-optimize="-O2 -GS- -Qfast_transcendentals -Qpar -Qpar-report:1"
ac_add_options --enable-jemalloc
ac_add_options --enable-shared-js
ac_add_options --disable-debug
ac_add_options --disable-tests 
ac_add_options --disable-mochitests
ac_add_options --enable-strip
ac_add_options --disable-updater
ac_add_options --disable-crashreporter
ac_add_options --disable-accessibility
ac_add_options --disable-parental-controls
ac_add_options --disable-maintenance-service
ac_add_options --disable-sandbox

I suppose I could also provide a binary if you wanted one, though I would probably have to recompile with branding turned off.

I hope I'll have better luck with dropping dead tonight and enjoying quite a few hours of sleep, so, if lucky, I will not be responding during that time.

@mattatobin

This comment has been minimized.

Show comment
Hide comment
@mattatobin

mattatobin Oct 19, 2016

Member

Tycho should be built with MozillaBuild 2.0 (2,1 may work but not sure about anything higher) that will solve your NSIS issues.

Member

mattatobin commented Oct 19, 2016

Tycho should be built with MozillaBuild 2.0 (2,1 may work but not sure about anything higher) that will solve your NSIS issues.

@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Oct 19, 2016

Member

I've been building with 2.1.0 thus far.
Also, the jxrlib should probably be placed in /media with all the other image decoder libs, but first things first would have to be a working implementation.

Member

wolfbeast commented Oct 19, 2016

I've been building with 2.1.0 thus far.
Also, the jxrlib should probably be placed in /media with all the other image decoder libs, but first things first would have to be a working implementation.

@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Oct 19, 2016

Member

We'd probably need a few test images to throw at this:

  • Lossy jxr
  • Lossless jxr
  • jxr with transparency
  • Progressive jxr (is this a thing, anyway?)
Member

wolfbeast commented Oct 19, 2016

We'd probably need a few test images to throw at this:

  • Lossy jxr
  • Lossless jxr
  • jxr with transparency
  • Progressive jxr (is this a thing, anyway?)
@mattatobin

This comment has been minimized.

Show comment
Hide comment
@mattatobin

mattatobin Oct 19, 2016

Member

The glue should live in image/decoders preferably as one source file and one header file

As Moonchild said, the lib itself should live in media/libjpeg-xr.

This may add an extra wrinkle because on Windows we are going back to building all media libs as a shared lib (gkmedias). So I am not sure what the impacts on the glue will be.

I wonder though, is the added depth of the directories for the lib required or could they be flattened at all? Are there any naming conflicts we may have? Also, are we exporting any headers that may need to be exported to a subdirectory of dist/includes. Have to be careful about collisions.

In any event, very cool, indeed!

Member

mattatobin commented Oct 19, 2016

The glue should live in image/decoders preferably as one source file and one header file

As Moonchild said, the lib itself should live in media/libjpeg-xr.

This may add an extra wrinkle because on Windows we are going back to building all media libs as a shared lib (gkmedias). So I am not sure what the impacts on the glue will be.

I wonder though, is the added depth of the directories for the lib required or could they be flattened at all? Are there any naming conflicts we may have? Also, are we exporting any headers that may need to be exported to a subdirectory of dist/includes. Have to be careful about collisions.

In any event, very cool, indeed!

@wolfbeast wolfbeast added the Assigned label Oct 19, 2016

@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Oct 19, 2016

Member

Setting this as assigned since @rhinoduck is obviously working on it.

Member

wolfbeast commented Oct 19, 2016

Setting this as assigned since @rhinoduck is obviously working on it.

@rhinoduck

This comment has been minimized.

Show comment
Hide comment
@rhinoduck

rhinoduck Oct 20, 2016

Contributor

Alas, this wasn't as much sleep as I was hoping for given that I skipped a night.

Thanks for the info, I'll switch to MB 2.1 for my next full tree rebuild to jump on board with the crowd.

..., but first things first would have to be a working implementation.

and

"We'd probably need a few test images to throw at this..."

Indeed, the first step was to get it into a basic working state so as to have a PoC and so that we have a base point we can return to if things start going awry. Now, we can safely start breaking stuff and start adding complexity :) Or rather removing... depends on the PoW really.

I welcome all the feedback on where you'd want things to live in the source tree and in what form. This is definitely something I don't have a mental map of yet. I concur with moving things around as one of the messy parts of this task was merging the directory structure which I already complained about in one of my not so nice comments.

# TODO: [rhinoduck] This will probably be reorganised. The directory structure
#       in moz.build files coming from the jxr example looked more like abstract
#       art than an attempt at placing things in sensible locations and actually
#       have them work at all.

I suppose when you talk about glue in the context of PM, you mean the nsJPEGXRDecoder.h and nsJPEGXRDecoder.cpp files? Because what currently lives in the jxrlib/jxrglue directory seems to be a different beast (read as: not Mozilla specific) which I'd rather keep as part of the library and as separate files.

As for exporting includes, JXRGlue.h is the only header that's directly included, and JXRMeta.h and windowsmediaphoto.h seem to be the only other headers it drags along with it as things stand now.

I will definitely be making changes to the directory structure of the lib, though I am wondering, is there any real advantage in flattening it completely? I'll draw inspiration from what's already present in media/, and come up with something I will consider reasonable; then, let's see if it's something you like or something you'd want me to change.

The name of the library is jxrlib and I am inclined to adding it to the media/ directory under this original name as to me, it is less confusing, and will cause no trouble should a libjpeg-xr pop up in the future (or should it already exist). Although, if you have a naming policy in place (or simply a preference) which favours adding it as libjpeg-xr, I will use that.

More testing images/scenarios will be coming as I get more familiar with the format. Keep throwing your thoughts and wishes at me, it is valuable information and it helps me with where I should look next and what I should focus on.

Also, if you have some early pointers or observations about what I should beware of for the Linux build, the sooner I am aware of these, the better. So far, I have been focusing on Windows only and, as even there it takes me over 3 hours to do a full source tree rebuild on my trusty old machine, I suspect that, in a VM, this will truly take a whole night. That's why I'd rather take precautions now than hunt silly things over several days later on.

Contributor

rhinoduck commented Oct 20, 2016

Alas, this wasn't as much sleep as I was hoping for given that I skipped a night.

Thanks for the info, I'll switch to MB 2.1 for my next full tree rebuild to jump on board with the crowd.

..., but first things first would have to be a working implementation.

and

"We'd probably need a few test images to throw at this..."

Indeed, the first step was to get it into a basic working state so as to have a PoC and so that we have a base point we can return to if things start going awry. Now, we can safely start breaking stuff and start adding complexity :) Or rather removing... depends on the PoW really.

I welcome all the feedback on where you'd want things to live in the source tree and in what form. This is definitely something I don't have a mental map of yet. I concur with moving things around as one of the messy parts of this task was merging the directory structure which I already complained about in one of my not so nice comments.

# TODO: [rhinoduck] This will probably be reorganised. The directory structure
#       in moz.build files coming from the jxr example looked more like abstract
#       art than an attempt at placing things in sensible locations and actually
#       have them work at all.

I suppose when you talk about glue in the context of PM, you mean the nsJPEGXRDecoder.h and nsJPEGXRDecoder.cpp files? Because what currently lives in the jxrlib/jxrglue directory seems to be a different beast (read as: not Mozilla specific) which I'd rather keep as part of the library and as separate files.

As for exporting includes, JXRGlue.h is the only header that's directly included, and JXRMeta.h and windowsmediaphoto.h seem to be the only other headers it drags along with it as things stand now.

I will definitely be making changes to the directory structure of the lib, though I am wondering, is there any real advantage in flattening it completely? I'll draw inspiration from what's already present in media/, and come up with something I will consider reasonable; then, let's see if it's something you like or something you'd want me to change.

The name of the library is jxrlib and I am inclined to adding it to the media/ directory under this original name as to me, it is less confusing, and will cause no trouble should a libjpeg-xr pop up in the future (or should it already exist). Although, if you have a naming policy in place (or simply a preference) which favours adding it as libjpeg-xr, I will use that.

More testing images/scenarios will be coming as I get more familiar with the format. Keep throwing your thoughts and wishes at me, it is valuable information and it helps me with where I should look next and what I should focus on.

Also, if you have some early pointers or observations about what I should beware of for the Linux build, the sooner I am aware of these, the better. So far, I have been focusing on Windows only and, as even there it takes me over 3 hours to do a full source tree rebuild on my trusty old machine, I suspect that, in a VM, this will truly take a whole night. That's why I'd rather take precautions now than hunt silly things over several days later on.

@mattatobin

This comment has been minimized.

Show comment
Hide comment
@mattatobin

mattatobin Oct 20, 2016

Member

Well.. you don't have to be burdened on build infra.. As I can complete builds in 30-40 minutes I can sure play tryserver for you on both Windows and Linux.. As for mozillaness of the file locations and stuff like that I kinda "own" the build system and am obsessed with tree structure so I can also help with that as well..

While we can do this here in the issue no problem if you want a more direct 2-way conversation you can always stop by our IRC channel. Up to you really..

Member

mattatobin commented Oct 20, 2016

Well.. you don't have to be burdened on build infra.. As I can complete builds in 30-40 minutes I can sure play tryserver for you on both Windows and Linux.. As for mozillaness of the file locations and stuff like that I kinda "own" the build system and am obsessed with tree structure so I can also help with that as well..

While we can do this here in the issue no problem if you want a more direct 2-way conversation you can always stop by our IRC channel. Up to you really..

@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Oct 20, 2016

Member

Obsessions aside, there's no real reason to flatten everything completely. I'd rather keep some hierarchy in it. Considering the jxrlib is laid out the way it is and it's a 3rd party library, we should not mess with its structure. That's just wholly unnecessary.

@mattatobin: The jxrglue/* is part of the jxrlib itself; it provides the exposed API AFAICT, and should live in the jxrlib location. The only thing we don't agree on is the actual location of jxrlib/* as a whole living in /image instead of /media.

Member

wolfbeast commented Oct 20, 2016

Obsessions aside, there's no real reason to flatten everything completely. I'd rather keep some hierarchy in it. Considering the jxrlib is laid out the way it is and it's a 3rd party library, we should not mess with its structure. That's just wholly unnecessary.

@mattatobin: The jxrglue/* is part of the jxrlib itself; it provides the exposed API AFAICT, and should live in the jxrlib location. The only thing we don't agree on is the actual location of jxrlib/* as a whole living in /image instead of /media.

@rhinoduck

This comment has been minimized.

Show comment
Hide comment
@rhinoduck

rhinoduck Oct 20, 2016

Contributor

Considering the jxrlib is laid out the way it is and it's a 3rd party library, we should not mess with its structure. That's just wholly unnecessary.

I actually came to the same conclusion and already have it that way in my local repo. The whole "everything in the image folder and various strange additional subfolders" thing came from the fact that it was used that way in the jxr example and their moz.build files, and from my lack of understanding at the time; I was just trying to follow what they were doing as it was the easiest starting point when trying to make it work.

Discussing things here seemed sufficient to me until now and I like that everything gets nicely archived in one place, but I do have a few questions on the side which are better suited for IRC so I will drop by eventually. My only issue with IRC is that I get very easily caught up in conversations which have nothing to do with the work that needs to be done; not that I don't enjoy them, it's just that there isn't much progress as a result :)

Contributor

rhinoduck commented Oct 20, 2016

Considering the jxrlib is laid out the way it is and it's a 3rd party library, we should not mess with its structure. That's just wholly unnecessary.

I actually came to the same conclusion and already have it that way in my local repo. The whole "everything in the image folder and various strange additional subfolders" thing came from the fact that it was used that way in the jxr example and their moz.build files, and from my lack of understanding at the time; I was just trying to follow what they were doing as it was the easiest starting point when trying to make it work.

Discussing things here seemed sufficient to me until now and I like that everything gets nicely archived in one place, but I do have a few questions on the side which are better suited for IRC so I will drop by eventually. My only issue with IRC is that I get very easily caught up in conversations which have nothing to do with the work that needs to be done; not that I don't enjoy them, it's just that there isn't much progress as a result :)

@mattatobin

This comment has been minimized.

Show comment
Hide comment
@mattatobin

mattatobin Oct 21, 2016

Member

I'm sorry you already had decoder cpp in decoders directory.. I must have been sleepy when I was poking at it.. Yeah seems the only thing is move the jxrlib code as-is to media and deal with gkmedias.

Member

mattatobin commented Oct 21, 2016

I'm sorry you already had decoder cpp in decoders directory.. I must have been sleepy when I was poking at it.. Yeah seems the only thing is move the jxrlib code as-is to media and deal with gkmedias.

rhinoduck added a commit to rhinoduck/Pale-Moon that referenced this issue Feb 8, 2017

Add JXR support to the browser
Can be enabled/disabled at runtime by toggling the 'media.jxr.enabled'
pref (disabled by default).

Two additional prefs are provided for testing purposes:
'media.jxr.autoaccept', and 'media.jxr.advertised_mime_type'
See comments in all.js for information on what these do.

This commit includes the MS OpenTech implementation of the decoder on
the browser side with some fixes applied; see the development in Pale
Moon GitGub issue MoonchildProductions#105 or my comments in the source code for more
information.
@rhinoduck

This comment has been minimized.

Show comment
Hide comment
@rhinoduck

rhinoduck Feb 8, 2017

Contributor

Created pull request #885 (I suppose the automatic cross-referencing would have been enough here, I might leave the links out next time). It includes the renaming I mentioned in my previous post; if you do not want it, I can rip it out.

The unsquashed commits can still be seen in the jxr-review branch if you want to follow the trail (but the latest additions were basically just the renaming).

Contributor

rhinoduck commented Feb 8, 2017

Created pull request #885 (I suppose the automatic cross-referencing would have been enough here, I might leave the links out next time). It includes the renaming I mentioned in my previous post; if you do not want it, I can rip it out.

The unsquashed commits can still be seen in the jxr-review branch if you want to follow the trail (but the latest additions were basically just the renaming).

@rhinoduck

This comment has been minimized.

Show comment
Hide comment
@rhinoduck

rhinoduck Feb 8, 2017

Contributor

but the latest additions were basically just the renaming

Actually, that is not true unless you've already looked today. Either way, the jxr-review branch shows things piece by piece if needed.

Contributor

rhinoduck commented Feb 8, 2017

but the latest additions were basically just the renaming

Actually, that is not true unless you've already looked today. Either way, the jxr-review branch shows things piece by piece if needed.

@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Feb 8, 2017

Member

Renaming JPEGXR and JPEG_XR to just JXR is fine - it's still clear what is being referred to since it's not ambiguous in our tree.
I'll merge #885 in and build a new unstable build today to have it included, and we can go from there.

Member

wolfbeast commented Feb 8, 2017

Renaming JPEGXR and JPEG_XR to just JXR is fine - it's still clear what is being referred to since it's not ambiguous in our tree.
I'll merge #885 in and build a new unstable build today to have it included, and we can go from there.

wolfbeast added a commit that referenced this issue Feb 8, 2017

Merge pull request #885 from rhinoduck/jxr-integration-pr
Add JXR support -- see #105 for details.
@trav90

This comment has been minimized.

Show comment
Hide comment
@trav90

trav90 Feb 12, 2017

Member

While in my (very limited) testing thus far it appears to be working, some versions of GCC throw almost 200 lines worth of warnings when compiling the JXRDecoder which you can see in this gist:

https://gist.github.com/trav90/a89f39afeb7650b3d52ac445c47354db

(I am only mentioning due to it being such a high number of warnings generated).

Member

trav90 commented Feb 12, 2017

While in my (very limited) testing thus far it appears to be working, some versions of GCC throw almost 200 lines worth of warnings when compiling the JXRDecoder which you can see in this gist:

https://gist.github.com/trav90/a89f39afeb7650b3d52ac445c47354db

(I am only mentioning due to it being such a high number of warnings generated).

@rhinoduck

This comment has been minimized.

Show comment
Hide comment
@rhinoduck

rhinoduck Feb 12, 2017

Contributor

That would be the MSOT code. As I stated above, I did not modify it beyond the fixes needed to make it compile and observably work so that some form of JXR support could be included now as requested. Though, at a glance, I don't see anything problematic the warnings point to except not checking the return values in err. Anyway, the not-so-easy-to-follow flow, state meaninglessly scattered across many variables, and the general notion of this being rushed and never finished and fully tested are exactly the reasons why I am already in the process of rewriting it piece by piece; that will do away with the warnings as well.

If they should be bothersome in the meantime, let me know, and I'll get rid off them.

Contributor

rhinoduck commented Feb 12, 2017

That would be the MSOT code. As I stated above, I did not modify it beyond the fixes needed to make it compile and observably work so that some form of JXR support could be included now as requested. Though, at a glance, I don't see anything problematic the warnings point to except not checking the return values in err. Anyway, the not-so-easy-to-follow flow, state meaninglessly scattered across many variables, and the general notion of this being rushed and never finished and fully tested are exactly the reasons why I am already in the process of rewriting it piece by piece; that will do away with the warnings as well.

If they should be bothersome in the meantime, let me know, and I'll get rid off them.

@trav90

This comment has been minimized.

Show comment
Hide comment
@trav90

trav90 Feb 12, 2017

Member

They aren't particularly bothersome to me, since (as you mention) this code is a bit rushed and not fully tested yet and it does appear to be working in my (so far) limited testing. I was just a little surprised to see a large increase in compiler warnings today, so just thought I'd mention it more as a FYI than anything.

Member

trav90 commented Feb 12, 2017

They aren't particularly bothersome to me, since (as you mention) this code is a bit rushed and not fully tested yet and it does appear to be working in my (so far) limited testing. I was just a little surprised to see a large increase in compiler warnings today, so just thought I'd mention it more as a FYI than anything.

@rhinoduck

This comment has been minimized.

Show comment
Hide comment
@rhinoduck

rhinoduck Feb 12, 2017

Contributor

Thanks for mentioning them, it would definitely be good not to have them pop up. I just didn't go looking for warnings given the current plan.

I was just a little surprised to see a large increase in compiler warnings today

This is actually what I mostly meant by bothersome; especially if you are inspecting the list of warnings regularly (Obviously, I didn't start doing so yet :). So, if you want them gone, just say so.

Contributor

rhinoduck commented Feb 12, 2017

Thanks for mentioning them, it would definitely be good not to have them pop up. I just didn't go looking for warnings given the current plan.

I was just a little surprised to see a large increase in compiler warnings today

This is actually what I mostly meant by bothersome; especially if you are inspecting the list of warnings regularly (Obviously, I didn't start doing so yet :). So, if you want them gone, just say so.

@trav90

This comment has been minimized.

Show comment
Hide comment
@trav90

trav90 Feb 12, 2017

Member

While I can live with them myself, I do think it would be best in the long run if they could be fixed (if nothing else to prevent users from asking about it once they start building 27.2.0 themselves).

Member

trav90 commented Feb 12, 2017

While I can live with them myself, I do think it would be best in the long run if they could be fixed (if nothing else to prevent users from asking about it once they start building 27.2.0 themselves).

@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Feb 23, 2017

Member

So, has anyone been able to give this a good test on a selection of sample images yet?
I gather the code itself is stable enough since it's been baking on trunk for a little while now; just need functional feedback for things mentioned in #105 (comment).

Member

wolfbeast commented Feb 23, 2017

So, has anyone been able to give this a good test on a selection of sample images yet?
I gather the code itself is stable enough since it's been baking on trunk for a little while now; just need functional feedback for things mentioned in #105 (comment).

@rhinoduck

This comment has been minimized.

Show comment
Hide comment
@rhinoduck

rhinoduck Feb 23, 2017

Contributor

This is what I used for testing:
http://rhinoduck.net/palemoon/github/issues/105/imgout/
(Only cloud and rock_transparent have transparecy. The more subdirectory contains the same images in various qualities.)

This I used to test the problem with transparent areas in tiled backgrounds being black:
http://rhinoduck.net/palemoon/github/issues/105/bgtest/

This I used to test whether disrupted connection/incomplete data no longer crashes the decoder:
http://rhinoduck.net/palemoon/github/issues/105/slow/
(The images under this folder are expected not to load; and the browser is expected to hold.)

What I did not test yet are the CMYK formats and embedded colour profiles.

It would be nice if we had a real-world gallery to test on; unfortunately, I don't know of any.

Contributor

rhinoduck commented Feb 23, 2017

This is what I used for testing:
http://rhinoduck.net/palemoon/github/issues/105/imgout/
(Only cloud and rock_transparent have transparecy. The more subdirectory contains the same images in various qualities.)

This I used to test the problem with transparent areas in tiled backgrounds being black:
http://rhinoduck.net/palemoon/github/issues/105/bgtest/

This I used to test whether disrupted connection/incomplete data no longer crashes the decoder:
http://rhinoduck.net/palemoon/github/issues/105/slow/
(The images under this folder are expected not to load; and the browser is expected to hold.)

What I did not test yet are the CMYK formats and embedded colour profiles.

It would be nice if we had a real-world gallery to test on; unfortunately, I don't know of any.

@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Mar 5, 2017

Member

I've been wanting to verify implementation with these links but your bgtest links are broken.

Member

wolfbeast commented Mar 5, 2017

I've been wanting to verify implementation with these links but your bgtest links are broken.

@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Mar 5, 2017

Member

@rhinoduck other images with different encodings all verify correctly, so considering this otherwise solved and ready for bounty payment. Please send your paypal e-mail address to moonchild@palemoon.org and we'll get that going.

Member

wolfbeast commented Mar 5, 2017

@rhinoduck other images with different encodings all verify correctly, so considering this otherwise solved and ready for bounty payment. Please send your paypal e-mail address to moonchild@palemoon.org and we'll get that going.

@rhinoduck

This comment has been minimized.

Show comment
Hide comment
@rhinoduck

rhinoduck Mar 16, 2017

Contributor

I've been wanting to verify implementation with these links but your bgtest links are broken.

The HTML documents in the directory the link leads to should have the cloud image tiled in the background, the link works for me and the background shows. Maybe I could have amended the HTML a bit more though (sorry) as the foreground image src and link is broken. What was the problem you had?


CMYK format and embedded colour profiles work, here are the images I tested with:

CMYK32
BGR24 + embedded colour profile
PBGRA32 (planar) + embedded colour profile
PBGRA32 (interleaved) + embedded colour profile

The colour profile (source) basically swaps reds and greens so the difference should be easily observable. I noticed that the MSOT code does not honour the gfx.color_management.mode pref, it just always behaves as if the value was 2 (color management is on for images which have embedded profiles); the rewrite already honours the pref.


I also (finally) realised that the (public) slow folder never actually worked (because of the interrupted transfers), so I created the slow-transfer folder which has the transfer speed capped at 32 kbps so that the progressive/chunked decoding can be triggered and seen.

During this, I noticed there was a difference between how the cloud image with interleaved and planar alpha shows up; the rewritten version does not have this problem.


Since the rewrite is almost done, I'd prefer finishing it to fixing the above-mentioned things in the MSOT code. But, besides those two, as of this moment I am not aware of any other issues in the currently commited Pale Moon side of the decoder, so this is a usable version as well; I just find the code rather hard to navigate.


Lastly, as a funny sidenote, Edge and IE advertise image/jxr, but they do not recognise the image/jxr MIME type when they receive a response from the server, they only decode what comes back as image/vnd.ms-photo; I really can't say I know what the though behind that was. Anyway, Pale Moon recognises both MIME types so that should cover all sane and insane variations.

Contributor

rhinoduck commented Mar 16, 2017

I've been wanting to verify implementation with these links but your bgtest links are broken.

The HTML documents in the directory the link leads to should have the cloud image tiled in the background, the link works for me and the background shows. Maybe I could have amended the HTML a bit more though (sorry) as the foreground image src and link is broken. What was the problem you had?


CMYK format and embedded colour profiles work, here are the images I tested with:

CMYK32
BGR24 + embedded colour profile
PBGRA32 (planar) + embedded colour profile
PBGRA32 (interleaved) + embedded colour profile

The colour profile (source) basically swaps reds and greens so the difference should be easily observable. I noticed that the MSOT code does not honour the gfx.color_management.mode pref, it just always behaves as if the value was 2 (color management is on for images which have embedded profiles); the rewrite already honours the pref.


I also (finally) realised that the (public) slow folder never actually worked (because of the interrupted transfers), so I created the slow-transfer folder which has the transfer speed capped at 32 kbps so that the progressive/chunked decoding can be triggered and seen.

During this, I noticed there was a difference between how the cloud image with interleaved and planar alpha shows up; the rewritten version does not have this problem.


Since the rewrite is almost done, I'd prefer finishing it to fixing the above-mentioned things in the MSOT code. But, besides those two, as of this moment I am not aware of any other issues in the currently commited Pale Moon side of the decoder, so this is a usable version as well; I just find the code rather hard to navigate.


Lastly, as a funny sidenote, Edge and IE advertise image/jxr, but they do not recognise the image/jxr MIME type when they receive a response from the server, they only decode what comes back as image/vnd.ms-photo; I really can't say I know what the though behind that was. Anyway, Pale Moon recognises both MIME types so that should cover all sane and insane variations.

@rhinoduck

This comment has been minimized.

Show comment
Hide comment
@rhinoduck

rhinoduck Mar 16, 2017

Contributor

A quick fix for some needless ugliness which can be applied now: #958

I don't know how far on the way to the door 27.2 is, but the rest should probably not be waited for; unless that is what you will want.

Contributor

rhinoduck commented Mar 16, 2017

A quick fix for some needless ugliness which can be applied now: #958

I don't know how far on the way to the door 27.2 is, but the rest should probably not be waited for; unless that is what you will want.

@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Mar 16, 2017

Member

27.2 is pretty much on the doorstep - I'll drop in the enhancement as a last-second addition, and the rewrite as proposed can be landed on trunk for the next release. I'll open a new issue for that with your description, since we have basic JXR support now.

Member

wolfbeast commented Mar 16, 2017

27.2 is pretty much on the doorstep - I'll drop in the enhancement as a last-second addition, and the rewrite as proposed can be landed on trunk for the next release. I'll open a new issue for that with your description, since we have basic JXR support now.

@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Mar 16, 2017

Member

The slow-transfer still doesn't work

Not Found

The requested URL /palemoon/github/issues/105/slow-transfer/bgtest/dogonatrain_large.jxr was not found on this server.
Member

wolfbeast commented Mar 16, 2017

The slow-transfer still doesn't work

Not Found

The requested URL /palemoon/github/issues/105/slow-transfer/bgtest/dogonatrain_large.jxr was not found on this server.
@wolfbeast

This comment has been minimized.

Show comment
Hide comment
@wolfbeast

wolfbeast Mar 16, 2017

Member

Since our support in MSOT fashion is pretty much complete, I'll enable jxr support by default. People can always switch it off if it causes issues for them.

Member

wolfbeast commented Mar 16, 2017

Since our support in MSOT fashion is pretty much complete, I'll enable jxr support by default. People can always switch it off if it causes issues for them.

@rhinoduck

This comment has been minimized.

Show comment
Hide comment
@rhinoduck

rhinoduck Mar 18, 2017

Contributor

The slow-transfer still doesn't work

The foreground image and the link are irrelevant to the bg test, they just remained there when I quickly reused the HTML; I am sorry if it caused confusion. I modified the documents and everything should be clear now.


A thought occurred to me today while outside wrt the issue which presented itself after slow rendering of the cloud image with planar alpha, and sure enough, that was it; It should have hit me immediately. Here is another little fix since it was easy to do: #964

Contributor

rhinoduck commented Mar 18, 2017

The slow-transfer still doesn't work

The foreground image and the link are irrelevant to the bg test, they just remained there when I quickly reused the HTML; I am sorry if it caused confusion. I modified the documents and everything should be clear now.


A thought occurred to me today while outside wrt the issue which presented itself after slow rendering of the cloud image with planar alpha, and sure enough, that was it; It should have hit me immediately. Here is another little fix since it was easy to do: #964

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.