Fix regression from #506: Only post-divide alpha for image writing

Eyescale · Nov 5, 2015 · 6707756 · 6707756
1 parent deddf3a
commit 6707756
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 65 deletions.
diff --git a/doc/Changelog.md b/doc/Changelog.md
@@ -3,11 +3,10 @@ Changelog {#Changelog}
 
 # git master
 
-* [506](https://github.com/Eyescale/Equalizer/pull/506):
-  eq::ResultImageListener::notifyNewImage() now reports post-divided alpha image
-* [506](https://github.com/Eyescale/Equalizer/pull/506):
-  Add eq::Image::postDivideAlpha() to fix premultiplied alpha images from
-  glReadPixels()
+# Release 1.10 (5-Nov-2015)
+
+* [508](https://github.com/Eyescale/Equalizer/pull/508):
+  Post-divide alpha from pixels in eq::Image::writeImage()
 * [504](https://github.com/Eyescale/Equalizer/pull/504):
   Let the OS choose the server port
 * [502](https://github.com/Eyescale/Equalizer/pull/500):

diff --git a/eq/detail/channel.ipp b/eq/detail/channel.ipp
@@ -123,10 +123,6 @@ public:
                                             channel.getObjectManager( )))
         {
             framebufferImage.finishReadback( channel.glewGetContext( ));
-
-            // glReadPixels with alpha has ARGB premultiplied format.
-            // Post-divide alpha for image saving etc.
-            framebufferImage.postDivideAlpha();
         }
     }
 

diff --git a/eq/image.cpp b/eq/image.cpp
@@ -169,7 +169,11 @@ namespace detail
 class Image
 {
 public:
-    Image() : type( eq::Frame::TYPE_MEMORY ), ignoreAlpha( false ) {}
+    Image()
+        : type( eq::Frame::TYPE_MEMORY )
+        , ignoreAlpha( false )
+        , hasPremultipliedAlpha( false )
+    {}
 
     /** The rectangle of the current pixel data. */
     PixelViewport pvp;
@@ -186,6 +190,8 @@ class Image
     /** Alpha channel significance. */
     bool ignoreAlpha;
 
+    bool hasPremultipliedAlpha;
+
     Attachment& getAttachment( const eq::Frame::Buffer buffer )
     {
         switch( buffer )
@@ -246,6 +252,7 @@ Image::~Image()
 void Image::reset()
 {
     _impl->ignoreAlpha = false;
+    _impl->hasPremultipliedAlpha = false;
     setPixelViewport( PixelViewport( ));
 }
 
@@ -675,6 +682,9 @@ void Image::_finishReadback( const Frame::Buffer buffer,
         return;
     }
 
+    if( memory.hasAlpha && buffer == Frame::BUFFER_COLOR )
+        _impl->hasPremultipliedAlpha = true;
+
     flags |= ( memory.hasAlpha ? 0 : EQ_COMPRESSOR_IGNORE_ALPHA );
 
     uint64_t outDims[4] = {0};
@@ -1154,13 +1164,51 @@ bool Image::writeImage( const std::string& filename,
     if( nPixels == 0 || memory.state != Memory::VALID )
         return false;
 
-    std::ofstream image( filename.c_str(), std::ios::out | std::ios::binary );
-    if( !image.is_open( ))
+    const unsigned char* data =
+         reinterpret_cast<const unsigned char*>( getPixelPointer( buffer ));
+
+    unsigned char* convertedData = nullptr;
+
+    // glReadPixels with alpha has ARGB premultiplied format: post-divide alpha
+    if( _impl->hasPremultipliedAlpha &&
+        getExternalFormat( buffer ) == EQ_COMPRESSOR_DATATYPE_BGRA )
     {
-        LBERROR << "Can't open " << filename << " for writing" << std::endl;
-        return false;
+        convertedData = new unsigned char[nPixels*4];
+        memcpy( convertedData, data, nPixels*4 );
+
+        uint32_t* rgbaData = reinterpret_cast< uint32_t* >( convertedData );
+        for( size_t i = 0; i < nPixels; ++i, ++rgbaData )
+        {
+            uint32_t& pixel = *rgbaData;
+            const uint32_t alpha = pixel >> 24;
+            if( alpha != 0 )
+            {
+                const uint32_t red = (pixel >> 16) & 0xff;
+                const uint32_t green = (pixel >> 8) & 0xff;
+                const uint32_t blue = pixel & 0xff;
+                pixel = (( alpha << 24 ) |
+                        (((255 * red) / alpha ) << 16 ) |
+                        (((255 * green) / alpha )  << 8 ) |
+                        ((255 * blue) / alpha ));
+            }
+        }
     }
 
+    const bool retVal = _writeImage( filename, buffer,
+                                     convertedData ? convertedData : data );
+    if( convertedData )
+        delete [] convertedData;
+    return retVal;
+}
+
+bool Image::_writeImage( const std::string& filename,
+                         const Frame::Buffer buffer,
+                         const unsigned char* data_ ) const
+{
+    const Memory& memory = _impl->getMemory( buffer );
+    const PixelViewport& pvp = memory.pvp;
+    const size_t nPixels = pvp.w * pvp.h;
+
     RGBHeader header;
     header.width  = pvp.w;
     header.height = pvp.h;
@@ -1219,10 +1267,6 @@ bool Image::writeImage( const std::string& filename,
     }
     LBASSERT( header.bytesPerChannel > 0 );
 
-    const uint8_t bpc = header.bytesPerChannel;
-    const uint16_t nChannels = header.depth;
-    const size_t depth = nChannels * bpc;
-
     // Swap red & blue where needed
     bool swapRB = false;
     switch( getExternalFormat( buffer ))
@@ -1238,24 +1282,31 @@ bool Image::writeImage( const std::string& filename,
             swapRB = true;
     }
 
+    const uint8_t bpc = header.bytesPerChannel;
+    const uint16_t nChannels = header.depth;
+    const size_t depth = nChannels * bpc;
+
     const boost::filesystem::path path( filename );
 #ifdef EQUALIZER_USE_OPENSCENEGRAPH
     if( path.extension() != ".rgb" )
     {
-        const unsigned char* data =
-             reinterpret_cast<const unsigned char*>( getPixelPointer( buffer ));
         osg::ref_ptr<osg::Image> osgImage = new osg::Image();
         osgImage->setImage( pvp.w, pvp.h, depth, getExternalFormat( buffer ),
                             swapRB ? GL_RGBA : GL_BGRA, GL_UNSIGNED_BYTE,
-                            const_cast< unsigned char* >( data ),
+                            const_cast< unsigned char* >( data_ ),
                             osg::Image::NO_DELETE );
         return osgDB::writeImageFile( *osgImage, filename );
     }
 #endif
 
-    const size_t nBytes = nPixels * depth;
-    const char* data = reinterpret_cast<const char*>( getPixelPointer( buffer));
+    std::ofstream image( filename.c_str(), std::ios::out | std::ios::binary );
+    if( !image.is_open( ))
+    {
+        LBERROR << "Can't open " << filename << " for writing" << std::endl;
+        return false;
+    }
 
+    const size_t nBytes = nPixels * depth;
     if( header.bytesPerChannel > 2 )
         LBWARN << static_cast< int >( header.bytesPerChannel )
                << " bytes per channel not supported by RGB spec" << std::endl;
@@ -1265,6 +1316,8 @@ bool Image::writeImage( const std::string& filename,
     image.write( reinterpret_cast<const char *>( &header ), sizeof( header ));
     header.convert();
 
+    const char* data = reinterpret_cast< const char* >( data_ );
+
     // Each channel is saved separately
     if( nChannels == 3 || nChannels == 4 )
     {
@@ -1648,39 +1701,6 @@ bool Image::getAlphaUsage() const
     return !_impl->ignoreAlpha;
 }
 
-void Image::postDivideAlpha()
-{
-    switch( getExternalFormat( Frame::BUFFER_COLOR ))
-    {
-    case EQ_COMPRESSOR_DATATYPE_BGRA:
-        break;
-    default:
-        LBERROR << "Unsupported image format for postDivideAlpha(): "
-                << getExternalFormat( Frame::BUFFER_COLOR ) << std::endl;
-        return;
-    }
-
-    uint32_t* data =
-         reinterpret_cast< uint32_t* >( getPixelPointer( Frame::BUFFER_COLOR ));
-    const PixelViewport& pvp = _impl->getMemory( Frame::BUFFER_COLOR ).pvp;
-
-    for( int32_t i = 0; i < pvp.w * pvp.h; ++i, ++data )
-    {
-        uint32_t& pixel = *data;
-        const uint32_t alpha = pixel >> 24;
-        if( alpha != 0 )
-        {
-            const uint32_t red = (pixel >> 16) & 0xff;
-            const uint32_t green = (pixel >> 8) & 0xff;
-            const uint32_t blue = pixel & 0xff;
-            pixel = (( alpha << 24 ) |
-                    (((255 * red) / alpha ) << 16 ) |
-                    (((255 * green) / alpha )  << 8 ) |
-                    ((255 * blue) / alpha ));
-        }
-    }
-}
-
 void Image::setOffset( int32_t x, int32_t y )
 {
     _impl->pvp.x = x;

diff --git a/eq/image.h b/eq/image.h
@@ -244,13 +244,6 @@ class Image
     /** @return true if alpha data can not be ignored. @version 1.0 */
     EQ_API bool getAlphaUsage() const;
 
-    /**
-     * Undo premultiplied alpha for ARGB image, e.g. the result of glReadPixels.
-     * Only implemented for external format EQ_COMPRESSOR_DATATYPE_BGRA.
-     * @version 1.10
-     */
-    EQ_API void postDivideAlpha();
-
     /**
      * Set the minimum quality after a full download-compression path.
      *
@@ -435,6 +428,8 @@ class Image
 
     void _finishReadback( const Frame::Buffer buffer, const GLEWContext* );
     bool _readbackZoom( const Frame::Buffer buffer, util::ObjectManager& om );
+    bool _writeImage( const std::string& filename, const Frame::Buffer buffer,
+                      const unsigned char* data ) const;
 };
 };
 #endif // EQ_IMAGE_H
diff --git a/eq/resultImageListener.h b/eq/resultImageListener.h
@@ -43,8 +43,6 @@ class ResultImageListener
      * Notify on new image, called from rendering thread in
      * Channel::frameViewFinish().
      *
-     * Since version 1.10, the image pixels are post-divided by alpha.
-     *
      * @param channel the destination channel
      * @param image the new image, valid only in the current frame
      * @version 1.9