From 0853752b5dea627305913596e5ee9a80751e2b34 Mon Sep 17 00:00:00 2001 From: walbourn_cp Date: Thu, 6 Jun 2013 16:22:07 -0700 Subject: [PATCH] DirectXTex: code review feedback --- DirectXTex/DirectXTexMipmaps.cpp | 281 +++++++++++++++---------------- DirectXTex/Filters.h | 7 +- 2 files changed, 142 insertions(+), 146 deletions(-) diff --git a/DirectXTex/DirectXTexMipmaps.cpp b/DirectXTex/DirectXTexMipmaps.cpp index 382ee1a..28e7071 100644 --- a/DirectXTex/DirectXTexMipmaps.cpp +++ b/DirectXTex/DirectXTexMipmaps.cpp @@ -366,18 +366,18 @@ HRESULT _ResizeSeparateColorAndAlpha( _In_ IWICImagingFactory* pWIC, _In_ IWICBi //--- determine when to use WIC vs. non-WIC paths --- -static HRESULT _UseWICFiltering( _In_ DXGI_FORMAT format, _In_ DWORD filter ) +static bool _UseWICFiltering( _In_ DXGI_FORMAT format, _In_ DWORD filter ) { if ( filter & TEX_FILTER_FORCE_NON_WIC ) { // Explicit flag indicates use of non-WIC code paths - return S_FALSE; + return false; } if ( filter & TEX_FILTER_FORCE_WIC ) { // Explicit flag to use WIC code paths, skips all the case checks below - return S_OK; + return true; } static_assert( TEX_FILTER_POINT == 0x100000, "TEX_FILTER_ flag values don't match TEX_FILTER_MASK" ); @@ -388,13 +388,13 @@ static HRESULT _UseWICFiltering( _In_ DXGI_FORMAT format, _In_ DWORD filter ) if ( filter & TEX_FILTER_WRAP ) { // WIC only supports 'clamp' semantics (MIRROR is equivalent to clamp for linear) - return S_FALSE; + return false; } if ( BitsPerColor(format) > 8 ) { // Avoid the WIC bitmap scaler when doing Linear filtering of XR/HDR formats - return S_FALSE; + return false; } break; @@ -402,21 +402,18 @@ static HRESULT _UseWICFiltering( _In_ DXGI_FORMAT format, _In_ DWORD filter ) if ( filter & ( TEX_FILTER_WRAP | TEX_FILTER_MIRROR ) ) { // WIC only supports 'clamp' semantics - return S_FALSE; + return false; } if ( BitsPerColor(format) > 8 ) { // Avoid the WIC bitmap scaler when doing Cubic filtering of XR/HDR formats - return S_FALSE; + return false; } break; - - default: - break; } - return S_OK; + return true; } @@ -2054,13 +2051,65 @@ HRESULT GenerateMipMaps( const Image& baseImage, DWORD filter, size_t levels, Sc return HRESULT_FROM_WIN32( ERROR_NOT_SUPPORTED ); } - HRESULT hr = _UseWICFiltering( baseImage.format, filter ); - if ( FAILED(hr) ) - return hr; + HRESULT hr; static_assert( TEX_FILTER_POINT == 0x100000, "TEX_FILTER_ flag values don't match TEX_FILTER_MASK" ); - if ( hr == S_FALSE ) + if ( _UseWICFiltering( baseImage.format, filter ) ) + { + //--- Use WIC filtering to generate mipmaps ----------------------------------- + switch(filter & TEX_FILTER_MASK) + { + case 0: + case TEX_FILTER_POINT: + case TEX_FILTER_FANT: // Equivalent to Box filter + case TEX_FILTER_LINEAR: + case TEX_FILTER_CUBIC: + { + static_assert( TEX_FILTER_FANT == TEX_FILTER_BOX, "TEX_FILTER_ flag alias mismatch" ); + + WICPixelFormatGUID pfGUID; + if ( _DXGIToWIC( baseImage.format, pfGUID, true ) ) + { + // Case 1: Base image format is supported by Windows Imaging Component + HRESULT hr = (baseImage.height > 1 || !allow1D) + ? mipChain.Initialize2D( baseImage.format, baseImage.width, baseImage.height, 1, levels ) + : mipChain.Initialize1D( baseImage.format, baseImage.width, 1, levels ); + if ( FAILED(hr) ) + return hr; + + return _GenerateMipMapsUsingWIC( baseImage, filter, levels, pfGUID, mipChain, 0 ); + } + else + { + // Case 2: Base image format is not supported by WIC, so we have to convert, generate, and convert back + assert( baseImage.format != DXGI_FORMAT_R32G32B32A32_FLOAT ); + ScratchImage temp; + HRESULT hr = _ConvertToR32G32B32A32( baseImage, temp ); + if ( FAILED(hr) ) + return hr; + + const Image *timg = temp.GetImage( 0, 0, 0 ); + if ( !timg ) + return E_POINTER; + + ScratchImage tMipChain; + hr = _GenerateMipMapsUsingWIC( *timg, filter, levels, GUID_WICPixelFormat128bppRGBAFloat, tMipChain, 0 ); + if ( FAILED(hr) ) + return hr; + + temp.Release(); + + return _ConvertFromR32G32B32A32( tMipChain.GetImages(), tMipChain.GetImageCount(), tMipChain.GetMetadata(), baseImage.format, mipChain ); + } + } + break; + + default: + return HRESULT_FROM_WIN32( ERROR_NOT_SUPPORTED ); + } + } + else { //--- Use custom filters to generate mipmaps ---------------------------------- TexMetadata mdata; @@ -2133,60 +2182,6 @@ HRESULT GenerateMipMaps( const Image& baseImage, DWORD filter, size_t levels, Sc return HRESULT_FROM_WIN32( ERROR_NOT_SUPPORTED ); } } - else - { - //--- Use WIC filtering to generate mipmaps ----------------------------------- - switch(filter & TEX_FILTER_MASK) - { - case 0: - case TEX_FILTER_POINT: - case TEX_FILTER_FANT: // Equivalent to Box filter - case TEX_FILTER_LINEAR: - case TEX_FILTER_CUBIC: - { - static_assert( TEX_FILTER_FANT == TEX_FILTER_BOX, "TEX_FILTER_ flag alias mismatch" ); - - WICPixelFormatGUID pfGUID; - if ( _DXGIToWIC( baseImage.format, pfGUID, true ) ) - { - // Case 1: Base image format is supported by Windows Imaging Component - HRESULT hr = (baseImage.height > 1 || !allow1D) - ? mipChain.Initialize2D( baseImage.format, baseImage.width, baseImage.height, 1, levels ) - : mipChain.Initialize1D( baseImage.format, baseImage.width, 1, levels ); - if ( FAILED(hr) ) - return hr; - - return _GenerateMipMapsUsingWIC( baseImage, filter, levels, pfGUID, mipChain, 0 ); - } - else - { - // Case 2: Base image format is not supported by WIC, so we have to convert, generate, and convert back - assert( baseImage.format != DXGI_FORMAT_R32G32B32A32_FLOAT ); - ScratchImage temp; - HRESULT hr = _ConvertToR32G32B32A32( baseImage, temp ); - if ( FAILED(hr) ) - return hr; - - const Image *timg = temp.GetImage( 0, 0, 0 ); - if ( !timg ) - return E_POINTER; - - ScratchImage tMipChain; - hr = _GenerateMipMapsUsingWIC( *timg, filter, levels, GUID_WICPixelFormat128bppRGBAFloat, tMipChain, 0 ); - if ( FAILED(hr) ) - return hr; - - temp.Release(); - - return _ConvertFromR32G32B32A32( tMipChain.GetImages(), tMipChain.GetImageCount(), tMipChain.GetMetadata(), baseImage.format, mipChain ); - } - } - break; - - default: - return HRESULT_FROM_WIN32( ERROR_NOT_SUPPORTED ); - } - } } _Use_decl_annotations_ @@ -2226,84 +2221,11 @@ HRESULT GenerateMipMaps( const Image* srcImages, size_t nimages, const TexMetada assert( baseImages.size() == metadata.arraySize ); - HRESULT hr = _UseWICFiltering( metadata.format, filter ); - if ( FAILED(hr) ) - return hr; + HRESULT hr; static_assert( TEX_FILTER_POINT == 0x100000, "TEX_FILTER_ flag values don't match TEX_FILTER_MASK" ); - if ( hr == S_FALSE ) - { - //--- Use custom filters to generate mipmaps ---------------------------------- - TexMetadata mdata2 = metadata; - mdata2.mipLevels = levels; - - DWORD filter_select = ( filter & TEX_FILTER_MASK ); - if ( !filter_select ) - { - // Default filter choice - filter_select = ( ispow2(metadata.width) && ispow2(metadata.height) ) ? TEX_FILTER_BOX : TEX_FILTER_LINEAR; - } - - switch( filter_select ) - { - case TEX_FILTER_BOX: - hr = _Setup2DMips( &baseImages[0], metadata.arraySize, mdata2, mipChain ); - if ( FAILED(hr) ) - return hr; - - for( size_t item = 0; item < metadata.arraySize; ++item ) - { - hr = _Generate2DMipsBoxFilter( levels, mipChain, item ); - if ( FAILED(hr) ) - mipChain.Release(); - } - return hr; - - case TEX_FILTER_POINT: - hr = _Setup2DMips( &baseImages[0], metadata.arraySize, mdata2, mipChain ); - if ( FAILED(hr) ) - return hr; - - for( size_t item = 0; item < metadata.arraySize; ++item ) - { - hr = _Generate2DMipsPointFilter( levels, mipChain, item ); - if ( FAILED(hr) ) - mipChain.Release(); - } - return hr; - - case TEX_FILTER_LINEAR: - hr = _Setup2DMips( &baseImages[0], metadata.arraySize, mdata2, mipChain ); - if ( FAILED(hr) ) - return hr; - - for( size_t item = 0; item < metadata.arraySize; ++item ) - { - hr = _Generate2DMipsLinearFilter( levels, filter, mipChain, item ); - if ( FAILED(hr) ) - mipChain.Release(); - } - return hr; - - case TEX_FILTER_CUBIC: - hr = _Setup2DMips( &baseImages[0], metadata.arraySize, mdata2, mipChain ); - if ( FAILED(hr) ) - return hr; - - for( size_t item = 0; item < metadata.arraySize; ++item ) - { - hr = _Generate2DMipsCubicFilter( levels, filter, mipChain, item ); - if ( FAILED(hr) ) - mipChain.Release(); - } - return hr; - - default: - return HRESULT_FROM_WIN32( ERROR_NOT_SUPPORTED ); - } - } - else + if ( _UseWICFiltering( metadata.format, filter ) ) { //--- Use WIC filtering to generate mipmaps ----------------------------------- switch(filter & TEX_FILTER_MASK) @@ -2376,6 +2298,77 @@ HRESULT GenerateMipMaps( const Image* srcImages, size_t nimages, const TexMetada return HRESULT_FROM_WIN32( ERROR_NOT_SUPPORTED ); } } + else + { + //--- Use custom filters to generate mipmaps ---------------------------------- + TexMetadata mdata2 = metadata; + mdata2.mipLevels = levels; + + DWORD filter_select = ( filter & TEX_FILTER_MASK ); + if ( !filter_select ) + { + // Default filter choice + filter_select = ( ispow2(metadata.width) && ispow2(metadata.height) ) ? TEX_FILTER_BOX : TEX_FILTER_LINEAR; + } + + switch( filter_select ) + { + case TEX_FILTER_BOX: + hr = _Setup2DMips( &baseImages[0], metadata.arraySize, mdata2, mipChain ); + if ( FAILED(hr) ) + return hr; + + for( size_t item = 0; item < metadata.arraySize; ++item ) + { + hr = _Generate2DMipsBoxFilter( levels, mipChain, item ); + if ( FAILED(hr) ) + mipChain.Release(); + } + return hr; + + case TEX_FILTER_POINT: + hr = _Setup2DMips( &baseImages[0], metadata.arraySize, mdata2, mipChain ); + if ( FAILED(hr) ) + return hr; + + for( size_t item = 0; item < metadata.arraySize; ++item ) + { + hr = _Generate2DMipsPointFilter( levels, mipChain, item ); + if ( FAILED(hr) ) + mipChain.Release(); + } + return hr; + + case TEX_FILTER_LINEAR: + hr = _Setup2DMips( &baseImages[0], metadata.arraySize, mdata2, mipChain ); + if ( FAILED(hr) ) + return hr; + + for( size_t item = 0; item < metadata.arraySize; ++item ) + { + hr = _Generate2DMipsLinearFilter( levels, filter, mipChain, item ); + if ( FAILED(hr) ) + mipChain.Release(); + } + return hr; + + case TEX_FILTER_CUBIC: + hr = _Setup2DMips( &baseImages[0], metadata.arraySize, mdata2, mipChain ); + if ( FAILED(hr) ) + return hr; + + for( size_t item = 0; item < metadata.arraySize; ++item ) + { + hr = _Generate2DMipsCubicFilter( levels, filter, mipChain, item ); + if ( FAILED(hr) ) + mipChain.Release(); + } + return hr; + + default: + return HRESULT_FROM_WIN32( ERROR_NOT_SUPPORTED ); + } + } } diff --git a/DirectXTex/Filters.h b/DirectXTex/Filters.h index 6f241ac..2b9df51 100644 --- a/DirectXTex/Filters.h +++ b/DirectXTex/Filters.h @@ -65,7 +65,7 @@ struct LinearFilter float weight1; }; -static void _CreateLinearFilter( _In_ size_t source, _In_ size_t dest, _In_ bool wrap, _Out_writes_(dest) LinearFilter* lf ) +inline void _CreateLinearFilter( _In_ size_t source, _In_ size_t dest, _In_ bool wrap, _Out_writes_(dest) LinearFilter* lf ) { assert( source > 0 ); assert( dest > 0 ); @@ -73,6 +73,8 @@ static void _CreateLinearFilter( _In_ size_t source, _In_ size_t dest, _In_ bool float scale = float(source) / float(dest); + // Mirror is the same case as clamp for linear + for( size_t u = 0; u < dest; ++u ) { float srcB = ( float(u) + 0.5f ) * scale + 0.5f; @@ -145,6 +147,7 @@ inline ptrdiff_t bounduvw( ptrdiff_t u, ptrdiff_t maxu, bool wrap, bool mirror ) } } + // Handles clamp, but also a safety factor for degenerate images for wrap/mirror u = std::min( u, maxu ); u = std::max( u, 0 ); @@ -160,7 +163,7 @@ struct CubicFilter float x; }; -static void _CreateCubicFilter( _In_ size_t source, _In_ size_t dest, _In_ bool wrap, bool mirror, _Out_writes_(dest) CubicFilter* cf ) +inline void _CreateCubicFilter( _In_ size_t source, _In_ size_t dest, _In_ bool wrap, bool mirror, _Out_writes_(dest) CubicFilter* cf ) { assert( source > 0 ); assert( dest > 0 );