New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for the simple lossless and lossy (non animated) WebP Format for #273 #795

Merged
merged 11 commits into from Sep 2, 2015

Conversation

Projects
None yet
2 participants
@raetiacorvus
Contributor

raetiacorvus commented Sep 1, 2015

This pull request contains simple lossless and simple lossy webp file support with the help of libwebp.

For now this means aseprite can open and save static (not animated) lossless and lossy webp files as long as they do not use the extended format. We still lack support for animation, color profile, EXIF and XMP. But probably only animation is going to be of interest for aseprite for now.

Animation will be implemented in a second step along with support for the extended format, exposing the missing features so aseprite could use them if any need arises.

Also a rudimentary save options dialog was implemented so user can save webp in lossless or lossy format. It was made quick and probably still needs some optimization for better UX.

screenshot from 2015-09-01 13-53-55

raetiacorvus added some commits Sep 1, 2015

implement simple non animation webp for #273
This includes lossless and lossy webp file format. For this reason a
save option dialog was added giving rudimentary options for saving to
the user.
@raetiacorvus

This comment has been minimized.

Show comment
Hide comment
@raetiacorvus

raetiacorvus Sep 1, 2015

Contributor

The travis builds failed do to the split_filename_tests bug. Is it possible to redo it without another pull request?

Contributor

raetiacorvus commented Sep 1, 2015

The travis builds failed do to the split_filename_tests bug. Is it possible to redo it without another pull request?

@dacap

This comment has been minimized.

Show comment
Hide comment
@dacap

dacap Sep 1, 2015

Member

Hi @sirblackheart, don't worry about travis. The PR looks good so far. I'll make some comments and test it soon.

Member

dacap commented Sep 1, 2015

Hi @sirblackheart, don't worry about travis. The PR looks good so far. I'll make some comments and test it soon.

@dacap

View changes

Show outdated Hide outdated data/widgets/webp_options.xml
@@ -0,0 +1,48 @@
<!-- ASEPRITE -->
<!-- Copyright (C) 2014, 2015 by David Capello -->

This comment has been minimized.

@dacap

dacap Sep 1, 2015

Member

You can put 2015 and your name here 👍 the same for the webp_format.cpp file.

@dacap

dacap Sep 1, 2015

Member

You can put 2015 and your name here 👍 the same for the webp_format.cpp file.

@dacap

View changes

Show outdated Hide outdated src/app/file/webp_format.cpp
try {
// Load the window to ask to the user the JPEG options he wants.
UniquePtr<ui::Window> window(app::load_widget<ui::Window>("webp_options.xml", "webp_options"));

This comment has been minimized.

@dacap

dacap Sep 1, 2015

Member

You can use the auto-generated widget here. The .h is generated in your_build_dir/src/app/generated_webp_options.h and you can use it in this way:

#include "generated_webp_options.h"
...
app::gen::WebpOptions window;
...

You can find here an example.

@dacap

dacap Sep 1, 2015

Member

You can use the auto-generated widget here. The .h is generated in your_build_dir/src/app/generated_webp_options.h and you can use it in this way:

#include "generated_webp_options.h"
...
app::gen::WebpOptions window;
...

You can find here an example.

@dacap

View changes

Show outdated Hide outdated src/app/file/webp_format.cpp
return false;
}
buf = (uint8_t*) malloc(len);

This comment has been minimized.

@dacap

dacap Sep 2, 2015

Member

You can use base_malloc() or a std::vector directly (the last one is preferred, you don't need to worry about base_free()).

@dacap

dacap Sep 2, 2015

Member

You can use base_malloc() or a std::vector directly (the last one is preferred, you don't need to worry about base_free()).

@dacap

View changes

Show outdated Hide outdated src/app/file/webp_format.cpp
FileOp* fop;
};
const std::pair<WebPEncodingError, std::string> enc_error_map_data[] = {

This comment has been minimized.

@dacap

dacap Sep 2, 2015

Member

I'm not sure about this array of pairs + a map. Is this just to show error messages? Could we have a function with a switch/case to convert WebPEncodingError codes to const char* strings (not even std::string)?

@dacap

dacap Sep 2, 2015

Member

I'm not sure about this array of pairs + a map. Is this just to show error messages? Could we have a function with a switch/case to convert WebPEncodingError codes to const char* strings (not even std::string)?

@dacap

View changes

Show outdated Hide outdated src/app/file/webp_format.cpp
//Helper functions instead of std::stoi because of missing c++11 on mac os x
template<typename T>
static inline std::string ToString(const T& v)

This comment has been minimized.

@dacap

dacap Sep 2, 2015

Member

Is this function needed?

@dacap

dacap Sep 2, 2015

Member

Is this function needed?

This comment has been minimized.

@raetiacorvus

raetiacorvus Sep 2, 2015

Contributor

Both the ProgressReport and FileWriter function are hooked into the libwebp encoder which then calls them while encoding. As soon as bytes to write are ready it directly writes them into the file. It also calls ProgressReport to update on its progress.

@raetiacorvus

raetiacorvus Sep 2, 2015

Contributor

Both the ProgressReport and FileWriter function are hooked into the libwebp encoder which then calls them while encoding. As soon as bytes to write are ready it directly writes them into the file. It also calls ProgressReport to update on its progress.

This comment has been minimized.

@dacap

dacap Sep 2, 2015

Member

Sorry, it looks like a GitHub UI problem. I clicked the ToString() line. It looks like nobody is using it (anyway you can use convert_to<std::string>() if needed)

@dacap

dacap Sep 2, 2015

Member

Sorry, it looks like a GitHub UI problem. I clicked the ToString() line. It looks like nobody is using it (anyway you can use convert_to<std::string>() if needed)

@dacap

View changes

Show outdated Hide outdated src/app/file/webp_format.cpp
}
template<typename T>
static inline T FromString(const std::string& str)

This comment has been minimized.

@dacap
@dacap
@dacap

This comment has been minimized.

Show comment
Hide comment
@dacap

dacap Sep 2, 2015

Member

I got the following warnings on VC 2013:

webp_format.cpp(128) : warning C4800: 'int' : forcing value to bool 'true' or 'false' (performance warning)
webp_format.cpp(320) : warning C4244: 'argument' : conversion from 'float' to 'int', possible loss of data
webp_format.cpp(320) : warning C4244: '=' : conversion from 'int' to 'float', possible loss of data
webp_format.cpp(328) : warning C4800: 'int' : forcing value to bool 'true' or 'false' (performance warning)
webp_format.cpp(330) : warning C4244: 'argument' : conversion from 'float' to 'int', possible loss of data
webp_format.cpp(338) : warning C4244: '=' : conversion from 'int' to 'float', possible loss of data
webp_format.cpp(344) : warning C4244: 'argument' : conversion from 'float' to 'int', possible loss of data

About other issue, I'm not an huge fan of auto keyword. I've used it for iterators mainly. Here I think we can use Image* and long in their respective cases.

Member

dacap commented Sep 2, 2015

I got the following warnings on VC 2013:

webp_format.cpp(128) : warning C4800: 'int' : forcing value to bool 'true' or 'false' (performance warning)
webp_format.cpp(320) : warning C4244: 'argument' : conversion from 'float' to 'int', possible loss of data
webp_format.cpp(320) : warning C4244: '=' : conversion from 'int' to 'float', possible loss of data
webp_format.cpp(328) : warning C4800: 'int' : forcing value to bool 'true' or 'false' (performance warning)
webp_format.cpp(330) : warning C4244: 'argument' : conversion from 'float' to 'int', possible loss of data
webp_format.cpp(338) : warning C4244: '=' : conversion from 'int' to 'float', possible loss of data
webp_format.cpp(344) : warning C4244: 'argument' : conversion from 'float' to 'int', possible loss of data

About other issue, I'm not an huge fan of auto keyword. I've used it for iterators mainly. Here I think we can use Image* and long in their respective cases.

raetiacorvus added some commits Sep 2, 2015

fix type conversion warn. & remove problem preset
fix type conversions by putting WebPOptions Class in a seperate file like
GifOptions and use Getter and Setter for better handling. In cases
needed explicit casting was used to supress those warings.

Additionaly the LAST WebPHint Option was removed as it does not work for
this situation.
@dacap

This comment has been minimized.

Show comment
Hide comment
@dacap

dacap Sep 2, 2015

Member

Looks great 👍 Thanks for your effort Gabriel! I will merge it. Do you think we should close #273?

Member

dacap commented Sep 2, 2015

Looks great 👍 Thanks for your effort Gabriel! I will merge it. Do you think we should close #273?

@dacap dacap merged commit c2a58dd into aseprite:master Sep 2, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@raetiacorvus

This comment has been minimized.

Show comment
Hide comment
@raetiacorvus

raetiacorvus Sep 2, 2015

Contributor

Thanks for having the patience to review my code. I think we should keep #273 open till i get the extended format implemented.

Contributor

raetiacorvus commented Sep 2, 2015

Thanks for having the patience to review my code. I think we should keep #273 open till i get the extended format implemented.

@raetiacorvus raetiacorvus deleted the raetiacorvus:webp-simple-static-support branch Sep 3, 2015

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