Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2083008:
Support ply-image splash screen display for ST1q (Closed)
|
Created:
10 years, 7 months ago by kkonda Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, cros.approvers_codeaurora.org, Micah C Base URL:
http://src.chromium.org/git/chromiumos.git Visibility:
Public. |
DescriptionSupport ply-image splash screen display for ST1q
Change-Id: Ib452052603331a8f76c285cc248e25ef2d0cc1fe
Patch Set 1 #
Total comments: 31
Patch Set 2 : Fix code review comments #
Total comments: 2
Patch Set 3 : Fix space-tab issues #Messages
Total messages: 16 (0 generated)
I am not sure if anyone else is working on this or not. If you know someone how is, please add them too. Thanks!!
Resending as I didnt join the cros-reviews mailing list earlier. Sorry for the spam! ------------------------- I am not sure if anyone else is working on this or not. If you know someone how is, please add them too. Thanks!!
http://codereview.chromium.org/2083008/diff/1/2 File src/third_party/ply-image/src/ply-frame-buffer.c (right): http://codereview.chromium.org/2083008/diff/1/2#newcode537 src/third_party/ply-image/src/ply-frame-buffer.c:537: int line, lines, width; declare line in narrowest scope http://codereview.chromium.org/2083008/diff/1/2#newcode555 src/third_party/ply-image/src/ply-frame-buffer.c:555: // Use bytes-per-pixel based on the depth of display instead of assuming 32-bit depth 80-column c-style comments http://codereview.chromium.org/2083008/diff/1/2#newcode567 src/third_party/ply-image/src/ply-frame-buffer.c:567: dst += (-vdiff / 2) * (buffer->row_stride * buffer->bytes_per_pixel); unneeded brackets http://codereview.chromium.org/2083008/diff/1/2#newcode570 src/third_party/ply-image/src/ply-frame-buffer.c:570: if (buffer->bytes_per_pixel != sizeof(*data)) We are handling a specific-case here: bytes_per_pixel == 2 So maybe have a nested if bytes_per_pixel == 2 and return an error if its not. http://codereview.chromium.org/2083008/diff/1/2#newcode572 src/third_party/ply-image/src/ply-frame-buffer.c:572: // Handle the 16 bpp case (specifically RGB 565 case) c-style comments http://codereview.chromium.org/2083008/diff/1/2#newcode574 src/third_party/ply-image/src/ply-frame-buffer.c:574: uint32_t *src_32_temp, src_32_value; declare variables in narrowest scope. src_16_temp should be declared inside the following for loop http://codereview.chromium.org/2083008/diff/1/2#newcode576 src/third_party/ply-image/src/ply-frame-buffer.c:576: // Allocate temporary row pixel data storage for 16 bpp displays c-style comments http://codereview.chromium.org/2083008/diff/1/2#newcode577 src/third_party/ply-image/src/ply-frame-buffer.c:577: src_16 = (uint16_t *) malloc (width * sizeof(*src_16)); What if you write directly to dst? http://codereview.chromium.org/2083008/diff/1/2#newcode579 src/third_party/ply-image/src/ply-frame-buffer.c:579: // Return if the malloc fails unneeded comment http://codereview.chromium.org/2083008/diff/1/2#newcode589 src/third_party/ply-image/src/ply-frame-buffer.c:589: while (src_16_temp != src_16_end) { What if you write this as a for loop? http://codereview.chromium.org/2083008/diff/1/2#newcode591 src/third_party/ply-image/src/ply-frame-buffer.c:591: *src_16_temp++ = (uint16_t)(((src_32_value & 0x00F80000) >> 8) | \ 80-column Alternatively, the code could be written as: (src_32_value >> 8) & 0xF800; Not sure which is easier to reason about. http://codereview.chromium.org/2083008/diff/1/2#newcode592 src/third_party/ply-image/src/ply-frame-buffer.c:592: ((src_32_value & 0x0000FC00) >> 5) | \ 80-column http://codereview.chromium.org/2083008/diff/1/2#newcode595 src/third_party/ply-image/src/ply-frame-buffer.c:595: memcpy (dst, (char *)src_16, (width * buffer->bytes_per_pixel)); unneeded cast and unneeded brackets around expression http://codereview.chromium.org/2083008/diff/1/2#newcode597 src/third_party/ply-image/src/ply-frame-buffer.c:597: // Skip to where the next data point to be displayed is c-style comment http://codereview.chromium.org/2083008/diff/1/2#newcode602 src/third_party/ply-image/src/ply-frame-buffer.c:602: free (src_16); alignment
Thanks Mandeep for the review. I uploaded a second patchset with almost all of your comments incorporated and put in reasons for those that I didnt. Please let me know what you think. http://codereview.chromium.org/2083008/diff/1/2 File src/third_party/ply-image/src/ply-frame-buffer.c (right): http://codereview.chromium.org/2083008/diff/1/2#newcode537 src/third_party/ply-image/src/ply-frame-buffer.c:537: int line, lines, width; On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > declare line in narrowest scope Done. http://codereview.chromium.org/2083008/diff/1/2#newcode555 src/third_party/ply-image/src/ply-frame-buffer.c:555: // Use bytes-per-pixel based on the depth of display instead of assuming 32-bit depth On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > 80-column > c-style comments Done. http://codereview.chromium.org/2083008/diff/1/2#newcode567 src/third_party/ply-image/src/ply-frame-buffer.c:567: dst += (-vdiff / 2) * (buffer->row_stride * buffer->bytes_per_pixel); On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > unneeded brackets Done. http://codereview.chromium.org/2083008/diff/1/2#newcode570 src/third_party/ply-image/src/ply-frame-buffer.c:570: if (buffer->bytes_per_pixel != sizeof(*data)) On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > We are handling a specific-case here: bytes_per_pixel == 2 > > So maybe have a nested if bytes_per_pixel == 2 and return an error if its not. Done. http://codereview.chromium.org/2083008/diff/1/2#newcode572 src/third_party/ply-image/src/ply-frame-buffer.c:572: // Handle the 16 bpp case (specifically RGB 565 case) On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > c-style comments Done. http://codereview.chromium.org/2083008/diff/1/2#newcode574 src/third_party/ply-image/src/ply-frame-buffer.c:574: uint32_t *src_32_temp, src_32_value; On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > declare variables in narrowest scope. > > src_16_temp should be declared inside the following for loop Done. http://codereview.chromium.org/2083008/diff/1/2#newcode576 src/third_party/ply-image/src/ply-frame-buffer.c:576: // Allocate temporary row pixel data storage for 16 bpp displays On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > c-style comments Done. http://codereview.chromium.org/2083008/diff/1/2#newcode577 src/third_party/ply-image/src/ply-frame-buffer.c:577: src_16 = (uint16_t *) malloc (width * sizeof(*src_16)); On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > What if you write directly to dst? The idea is convert the whole row of pixel data and then memcpy the whole row. Leaving it as is for now but please let me know if you would like me to change it in any way. http://codereview.chromium.org/2083008/diff/1/2#newcode579 src/third_party/ply-image/src/ply-frame-buffer.c:579: // Return if the malloc fails On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > unneeded comment Done. http://codereview.chromium.org/2083008/diff/1/2#newcode589 src/third_party/ply-image/src/ply-frame-buffer.c:589: while (src_16_temp != src_16_end) { On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > What if you write this as a for loop? Done. http://codereview.chromium.org/2083008/diff/1/2#newcode591 src/third_party/ply-image/src/ply-frame-buffer.c:591: *src_16_temp++ = (uint16_t)(((src_32_value & 0x00F80000) >> 8) | \ On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > 80-column > > Alternatively, the code could be written as: > > (src_32_value >> 8) & 0xF800; > > Not sure which is easier to reason about. Edited to remain with the 80 column limit. I left the code as is for now - let me know if you think otherwise. http://codereview.chromium.org/2083008/diff/1/2#newcode592 src/third_party/ply-image/src/ply-frame-buffer.c:592: ((src_32_value & 0x0000FC00) >> 5) | \ On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > 80-column Done. http://codereview.chromium.org/2083008/diff/1/2#newcode595 src/third_party/ply-image/src/ply-frame-buffer.c:595: memcpy (dst, (char *)src_16, (width * buffer->bytes_per_pixel)); On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > unneeded cast and unneeded brackets around expression Done. http://codereview.chromium.org/2083008/diff/1/2#newcode597 src/third_party/ply-image/src/ply-frame-buffer.c:597: // Skip to where the next data point to be displayed is On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > c-style comment Done. http://codereview.chromium.org/2083008/diff/1/2#newcode602 src/third_party/ply-image/src/ply-frame-buffer.c:602: free (src_16); On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > alignment Done.
Thanks for fixing this. Just a few minor nits. http://codereview.chromium.org/2083008/diff/1/2 File src/third_party/ply-image/src/ply-frame-buffer.c (right): http://codereview.chromium.org/2083008/diff/1/2#newcode577 src/third_party/ply-image/src/ply-frame-buffer.c:577: src_16 = (uint16_t *) malloc (width * sizeof(*src_16)); On 2010/05/20 19:30:42, kkonda wrote: > On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > > What if you write directly to dst? > The idea is convert the whole row of pixel data and then memcpy the whole row. > Leaving it as is for now but please let me know if you would like me to change > it in any way. > By avoiding the malloc, you avoid an operation that can fail. With no malloc, there are no failure cases. Nice property. Also, malloc is expensive. memcpy is not atomic so whether you copy a pixel at a time or a single row via memcpy shouldn't matter. But I leave it to your best judgment. http://codereview.chromium.org/2083008/diff/6001/7001#newcode588 src/third_party/ply-image/src/ply-frame-buffer.c:588: white-space (this code is spaces and not tabs) http://codereview.chromium.org/2083008/diff/6001/7001#newcode593 src/third_party/ply-image/src/ply-frame-buffer.c:593: ((src_32_value & 0x000000F8) >> 3)); white-space (this code is spaces and not tabs)
> http://codereview.chromium.org/2083008/diff/1/2#newcode577 > src/third_party/ply-image/src/ply-frame-buffer.c:577: src_16 = (uint16_t *) > malloc (width * sizeof(*src_16)); > On 2010/05/20 19:30:42, kkonda wrote: > > On 2010/05/15 15:16:35, Mandeep Singh Baines wrote: > > > What if you write directly to dst? > > The idea is convert the whole row of pixel data and then memcpy the whole row. > > Leaving it as is for now but please let me know if you would like me to change > > it in any way. > > > > By avoiding the malloc, you avoid an operation that can fail. With no malloc, > there are no failure cases. Nice property. > > Also, malloc is expensive. > > memcpy is not atomic so whether you copy a pixel at a time or a single row via > memcpy shouldn't matter. > > But I leave it to your best judgment. The reason for doing it this way is to take advantage of the neonized memcpy - copying 2 bytes a time would be not efficient. Thanks for the quick review. I will fix the space/tabs issue and upload another patchset.
A good solution here could be to define a local array of 128 or so pixels and use that as your copy buffer instead of one that is the width of the frame buffer. That would avoid the malloc and allow you to take advantage of the NEON optimized memcpy.
On 2010/05/21 17:55:01, robotboy wrote: > A good solution here could be to define a local array of 128 or so pixels and > use that as your copy buffer instead of one that is the width of the frame > buffer. That would avoid the malloc and allow you to take advantage of the NEON > optimized memcpy. The fact that the buffer is known to be large enough for the row makes the loop structure optimal for icache coherency. This could be done with a stack array with #define MAX_LINE_WIDTH 1366. The splash screen is using valuable cpu while we want X server to start asap, so that really does matter. Please advise asap if you'd like a further change, otherwise would be great to begin the weekend with a proper splash. BTW the single-row malloc is quite unlikely to fail considering png buffer malloc already succeeded.
LGTM (but I think this is pre-mature optimization). kkonda@codeaurora.org (kkonda@codeaurora.org) wrote: > On 2010/05/21 17:55:01, robotboy wrote: >> A good solution here could be to define a local array of 128 or so >> pixels and >> use that as your copy buffer instead of one that is the width of the frame >> buffer. That would avoid the malloc and allow you to take advantage of >> the > NEON >> optimized memcpy. > > The fact that the buffer is known to be large enough for the row makes > the loop > structure optimal for icache coherency. This could be done with a stack > array > with #define MAX_LINE_WIDTH 1366. The splash screen is using valuable cpu > while > we want X server to start asap, so that really does matter. > I'm not sure the memcpy buys you anything. You're doing a byte for byte copy anyway. Can you run some profiles to see if this is really buying you anything. If you see a dramatic improvement, LGTM. > Please advise asap if you'd like a further change, otherwise would be > great to > begin the weekend with a proper splash. > > BTW the single-row malloc is quite unlikely to fail considering png buffer > malloc already succeeded. > Keep in mind, there are other processes on the system running concurrently, allocating memory. I guess I've just been burned too many times when I've tried to play odds. A few months down the line I'll get a bug report about something that happens once a week or once a month. It takes me a week to track it down and then I promise myself I'll code defensively from now on. Maybe you're luck is better than mine. > > http://codereview.chromium.org/2083008/show
LGTM I misunderstood what was going on in the code. Embarrassed:(
no worries. does the ebuild rev need to be bumped also?
tbourget@codeaurora.org (tbourget@codeaurora.org) wrote: > no worries. > > does the ebuild rev need to be bumped also? > Yes. The ebuild should be revved after any source change. > http://codereview.chromium.org/2083008/show
> Yes. The ebuild should be revved after any source change. Thanks Mandeep. Should I go ahead and submit a patch for revving the ebuild? Thanks, Krishna
kkonda@codeaurora.org (kkonda@codeaurora.org) wrote: >> Yes. The ebuild should be revved after any source change. > > Thanks Mandeep. Should I go ahead and submit a patch for revving the ebuild? > Yes, please. > > Thanks, > Krishna > > > http://codereview.chromium.org/2083008/show
The ebuild rev change is uploaded and the link is http://codereview.chromium.org/2323002/show Note (to whoever merges this in) Please make sure that the issue 2323002 gets merged at the same time as this. Thanks! On 2010/05/25 14:33:26, Mandeep Singh Baines wrote: > mailto:kkonda@codeaurora.org (mailto:kkonda@codeaurora.org) wrote: > >> Yes. The ebuild should be revved after any source change. > > > > Thanks Mandeep. Should I go ahead and submit a patch for revving the ebuild? > > > > Yes, please. > > > > > Thanks, > > Krishna > > > > > > http://codereview.chromium.org/2083008/show >
I was wondering when this will be merged. If there is anything pending on my end, please let me know. Thanks, Krishna On 2010/05/28 03:35:55, kkonda wrote: > The ebuild rev change is uploaded and the link is > http://codereview.chromium.org/2323002/show > > Note (to whoever merges this in) > Please make sure that the issue 2323002 gets merged at the same time as this. > > > Thanks! > > On 2010/05/25 14:33:26, Mandeep Singh Baines wrote: > > mailto:kkonda@codeaurora.org (mailto:kkonda@codeaurora.org) wrote: > > >> Yes. The ebuild should be revved after any source change. > > > > > > Thanks Mandeep. Should I go ahead and submit a patch for revving the ebuild? > > > > > > > Yes, please. > > > > > > > > Thanks, > > > Krishna > > > > > > > > > http://codereview.chromium.org/2083008/show > > |
