10231 loader fb-putimage should support image resizing and placement

Review Request #1370 — Created Jan. 15, 2019 and submitted

citrus
illumos-gate
master
10231
26778c1...
general
10231 loader fb-putimage should support image resizing and placement


  • 0
  • 0
  • 3
  • 6
  • 9
Description From Last Updated
tsoome
  1. Ship It!
  2. 
      
rm
  1. 
      
  2. usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)
     
     
     
     
     
     
    Is this trying to make sure if fits within a uint16_t, if so, use UINT16_MAX.
    1. Yes, effectively. It's checking that the fixed point arithmetic will not overflow the integer part. I can use UINT16_MAX instead (and if anyone ever tries to use an image with that many pixels I'm sure they'll hit other problems first)

  3. usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)
     
     
    So, this will disable scaling, but what if it doesn't actually fit in the framebuffer? I'm not sure I see anything that deals with that case.
    1. If it doesn't fit then the backend routine - gfx_fb_cons_display() - will just not draw it on the screen. Although I've included some basic sanity checks in this function there does not seem to be any point in duplicating the checks from the backend.

    2. Can you make a comment to note this, otherwise it's quite confusing to know that is the case and the fact that we're doing scaling here and there is probably going to be lost.
    3. Although it's safe to draw off the screen, I've split the coordinate check in two so that the bottom right position is checked after the calculations. I think that's a better approach.

  4. usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)
     
     
     
     
     
     
     
    Why exactly do we only care about this now if the flag isn't set? Also, since the block is multiple lines (though only one statement) can we put parens around it?
    1. tem_image_display() marks a rectangular area of the screen so that scrolling is handled properly (the image is copied up when the screen is scrolled). The new flag allows this to be skipped meaning that you can put an image at the top right of the screen, say, and have it persist while the text scrolls (if you wish).

      Braces added.

    2. There is a bit more about scrolling. It surely is my bug not having good comment there... The original tem (and what we still have in our kernel) is built to work on small resolutions and not optimized for large screens - the operations are performed with full lines etc. To get screen to perform, currently in loader, we do build screen "map" with chars and attributes, and we will only copy differences (in simplest form atm -- we only check line tails). For images, I did add extra attribute in that map, and we always do copy the image areas. This optimiztion is done in terms of terminal char areas.

      And yes, surely we can interpret this optimization in more than one way:)

  5. usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)
     
     
     
    Where does 16 come from in these expressions?
    1. I'm doing unsigned fixed point arithmetic and using the top 16 bits of the 32-bit value for the integer part and the lower 16 bits for the fractional part. To store an integer (png->width - 1) in this scheme, it needs to be shifted 16 bits left and it can then be divided by (fwidth - 1) to establish the number of pixels across in the source image that correspond to a single pixel in the target.

    2. I think it's worth a comment that explains what's going on here as it's pretty hard to parse initialy that this is what we're doing.

  6. usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)
     
     
     
    Please ensure that this is being built with c99 if you're going to do declarations here.
    1. It is built with:

      /opt/gcc-4.4.4//bin/gcc -Os -fPIC -ffreestanding -Wformat -msoft-float -mno-mmx -mno-3dnow -mno-sse2 -mno-sse3 -mno-sse -mno-avx -fshort-wchar -mno-aes -std=gnu99 -Wno-pointer-sign -Wno-empty-body -m64 -D_STANDALONE -DEFI -nostdinc -I. ... -c ../../../common/gfx_fb.c
      

      as is the rest of loader.

    2. Yes, the entire loader is intended to be built as c99. I still have patch waiting to re-organize all makefiles and collect the build options/arguments to one single location.

  7. usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)
     
     
    Where did 14 come from?
    1. I'm working out the contributions to each pixel value by combining adjacent pixels, assigning each a proportion of a total of 128
      - effectively pixel1 * X + pixel2 * (128 - X) I do this for two pairs of pixels then I combine the two pairs in the same way. To get back to the integer result, I need to divide again by (128 * 128) which is achieved by shifting 14 bits right. Doing the shift once, at the end, provides greater precision.

    2. Can you please elaborate in a comment about this? That way someone else reading it won't find it quite so magical either.

  8. 
      
citrus
rm
  1. 
      
  2. usr/src/boot/sys/boot/common/gfx_fb.c (Diff revisions 1 - 2)
     
     
    In the past you were doing >= 1 << 16, so shouldn't this be '> UINT16_MAX'?
    1. Yes, fixed, thanks.

  3. 
      
citrus
domag02
  1. Format string signedness mismatch.
  2. usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 3)
     
     

    This line should be:

            printf("Image %ux%u -> %ux%u @%ux%u\n",
    
  3. 
      
citrus
tsoome
  1. Ship It!
  2. 
      
domag02
  1. Ship It!
  2. 
      
citrus
rm
  1. Ship It!
  2. 
      
citrus
citrus
citrus
tsoome
  1. Ship It!
  2. 
      
domag02
  1. I'm not sure about that this is a real issue, but now every value accepted between in range of n * UINT32_MAX + 0 and n * UINT32_MAX + gfx_fb.framebuffer_common.framebuffer_height (similar rules applies to x2, y1, x1 and f variables, too) when this code compiled on LP64 datamodel.

    Drop this issue if you think it's not important.

  2. usr/src/common/ficl/loader.c (Diff revision 6)
     
     

    When the used datamodel is LP64, implicit type conversion can happen (variables gets only the lower 32bit of the original values).
    ficlStackPopUnsigned() return type is ficlUnsigned, wich is typedef-ed as uint32_t or uint64_t on LP64 (see in /usr/src/common/ficl/ficlplatform/unix.h, line 58).

    1. Taking only the bottom 32-bits in that case is fine and consistent with the other fb-* functions.

  3. 
      
rm
  1. Ship It!
  2. 
      
citrus
Review request changed

Status: Closed (submitted)

Loading...