Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make basis_transcoder.{js,wasm} substantially smaller #7

Merged
merged 5 commits into from
May 23, 2019
Merged

Make basis_transcoder.{js,wasm} substantially smaller #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a52a31e
Rebuild basis_transcoder with new Emscripten (emsdk 1.38.31)
zeux May 23, 2019
af8f9f8
Remove assert(x) hack in favor of NDEBUG
zeux May 23, 2019
6fa7476
Remove std::iostream from the transcoders
zeux May 23, 2019
f5d5029
Make codec options externally configurable and remove BC7 support
zeux May 23, 2019
c2430c3
Use emmalloc instead of dlmalloc to save a bit more space
zeux May 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions transcoder/basisu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,6 @@
#include <assert.h>
#include <random>

#if defined(__EMSCRIPTEN__) && !defined(_DEBUG) && !defined(DEBUG)
// HUGE HACK: I've been unable to disable assertions using emcc -O2 -s ASSERTIONS=0, no idea why.
// We definitely don't want them enabled in release.
#undef assert
#define assert(x) ((void)0)
#endif

#ifdef max
#undef max
#endif
Expand Down
24 changes: 19 additions & 5 deletions transcoder/basisu_transcoder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,25 @@
// Set to 1 for fuzz testing. This will disable all CRC16 checks on headers and compressed data.
#define BASISU_NO_HEADER_OR_DATA_CRC16_CHECKS 0

#define BASISD_SUPPORT_DXT1 1
#define BASISD_SUPPORT_DXT5A 1
#define BASISD_SUPPORT_BC7 1
#define BASISD_SUPPORT_PVRTC1 1
#define BASISD_SUPPORT_ETC2_EAC_A8 1
#ifndef BASISD_SUPPORT_DXT1
#define BASISD_SUPPORT_DXT1 1
#endif

#ifndef BASISD_SUPPORT_DXT5A
#define BASISD_SUPPORT_DXT5A 1
#endif

#ifndef BASISD_SUPPORT_BC7
#define BASISD_SUPPORT_BC7 1
#endif

#ifndef BASISD_SUPPORT_PVRTC1
#define BASISD_SUPPORT_PVRTC1 1
#endif

#ifndef BASISD_SUPPORT_ETC2_EAC_A8
#define BASISD_SUPPORT_ETC2_EAC_A8 1
#endif

#define BASISD_WRITE_NEW_BC7_TABLES 0
#define BASISD_WRITE_NEW_DXT1_TABLES 0
Expand Down
2 changes: 1 addition & 1 deletion webgl/transcoder/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@ if (EMSCRIPTEN)
set_target_properties(basis_transcoder.js PROPERTIES
OUTPUT_NAME "basis_transcoder"
SUFFIX ".js"
LINK_FLAGS "--bind -s ALLOW_MEMORY_GROWTH=1 -O3 -s ASSERTIONS=0")
LINK_FLAGS "--bind -s ALLOW_MEMORY_GROWTH=1 -O3 -s ASSERTIONS=0 -s MALLOC=emmalloc -DNDEBUG -DBASISD_SUPPORT_BC7=0")
endif()
3 changes: 0 additions & 3 deletions webgl/transcoder/basis_wrappers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
#include "basisu_transcoder.h"
#include <emscripten/bind.h>

#include <iostream>

using namespace emscripten;
using namespace basist;

Expand Down Expand Up @@ -39,7 +37,6 @@ struct basis_file

if (!m_transcoder.validate_header(m_file.data(), m_file.size())) {
m_file.clear();
std::cerr << "Invalid Basis header" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some sort of error reporting is desirable - at least a bool isValid() method returning (m_magic == MAGIC).

Copy link
Contributor Author

@zeux zeux May 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that error handling can/should be performed by checking the result of startTranscoding() call - the extra console output may be valuable for debugging but not as part of the production .wasm binary.

For debugging, you can build the transcoder with BASISU_DEVEL_MESSAGES option which will emit detailed error messages indicating what specifically went wrong with the header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separately, I think m_magic variable and all associated code can be removed based on the fact that every method of transcoder is doing quick header validation which should be ~as strong as the code in this file; not that big of a deal but that seems cleaner. It will also make it so that JS code can call startTranscoding and other functions without hitting assertions in debug builds for malformed files.

Copy link

@austinEng austinEng May 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we can remove the magic. This is old code when we weren't using Embind and had a more C-like api. It was possible to pass a raw pointer to any of the functions. Now that we have a proper BasisFile constructor, we could just have the constructor throw a Javascript exception (or have a createBasisFile function return undefined). That way none of the member functions are accessible unless you first make a valid BasisFile. This plus the quick header validation should be sufficient.

}

// Initialized after validation
Expand Down
2 changes: 1 addition & 1 deletion webgl/transcoder/build/basis_transcoder.js
View file
Open in desktop

Large diffs are not rendered by default.

Binary file modified webgl/transcoder/build/basis_transcoder.wasm
View file
Open in desktop
Binary file not shown.