Menu

#730 Recent colorspace conversion change

v1.0_(example)
closed-fixed
None
5
2023-11-07
2023-11-06
No

Hello Bob, I have a concern about the recent commit https://sourceforge.net/p/graphicsmagick/code/ci/8e342d89838cd3649c02b3725703050bfa69391f/

In integer arithmetic, the (foo + 1)/2 paradigm is used for rounding (and works for positive numbers only mind you).

When you switched to floats now, the "+ 1" is now actually introducing a bias, and thus, an error, no?

Discussion

  • Bob Friesenhahn

    Bob Friesenhahn - 2023-11-06

    Thank you for double-checking the code. It is easy to introduce an error.

    With this small test program I do not see a severe issue with results:

    #include <stdio.h>
    
    #if !defined(QuantumDepth)
    #  define QuantumDepth  16
    #endif
    
    #if (QuantumDepth == 8)
    #  define MaxMap  255U
    #  define MaxMapFloat 255.0f
    #  define MaxMapDouble 255.0
    #elif (QuantumDepth == 16)
    #  define MaxMap 65535U
    #  define MaxMapFloat 65535.0f
    #  define MaxMapDouble 65535.0
    #elif (QuantumDepth == 32)
    #  define MaxMap 65535U
    #  define MaxMapFloat 65535.0f
    #  define MaxMapDouble 65535.0
    #endif
    
    int main(int argc, char* argv[])
    {
      (void) argc;
      (void) argv;
    
      float int_round = ((MaxMap+1)/2);
      float float_round = (((float) MaxMap+1)/2);
    
      printf("int round = %g, float round = %g\n",
             int_round, float_round);
    
      return 0;
    }
    

    since I see these results from the program:

    % gcc -DQuantumDepth=8 -Wall -O3 -o rounding rounding.c
    % ./rounding                                           
    int round = 128, float round = 128
    % gcc -DQuantumDepth=16 -Wall -O3 -o rounding rounding.c
    % ./rounding                                            
    int round = 32768, float round = 32768
    % gcc -DQuantumDepth=32 -Wall -O3 -o rounding rounding.c
    % ./rounding                                            
    int round = 32768, float round = 32768
    

    I am reminded that there is a MaxMapFloat value which could have been used.
    My edits were a reaction due to using Visual Studio 2022's code analyzer to discover issues.

     
    • Milos Komarcevic

      Thanks for following up Bob. While it is true there is no difference in this particular case with odd constant MaxMap, I feel the code just reads a bit weird and the intent is no longer clearly expressed after the change.

      For future maintainability, may I suggest reverting back (or perhaps casting to float after the int rounding paradigm as (float)((MaxMap+1)/2) if all you want is to avoid VS warnings), or simply use ceilf(MaxMapFloat/2)?

       
  • Bob Friesenhahn

    Bob Friesenhahn - 2023-11-07
    • assigned_to: Bob Friesenhahn
     
  • Bob Friesenhahn

    Bob Friesenhahn - 2023-11-07

    I am not sure if the VS Analyzer will still be happy since it may (stupidly) be complaining about the MaxMap+1. It is too painful to verify that at the moment. Regardless, moving the cast seems fine to me.

     
  • Bob Friesenhahn

    Bob Friesenhahn - 2023-11-07
    • status: open --> closed-fixed
     
  • Bob Friesenhahn

    Bob Friesenhahn - 2023-11-07

    Concern is addressed by Mercurial changeset 17288:7e7556380183. Thanks for inspecting the commits!

     
    • Milos Komarcevic

      Thanks!

       

Log in to post a comment.

Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.