|
|
Created:
9 years, 8 months ago by marcheu Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org Visibility:
Public. |
DescriptionAdd gamma support to ply-image.
BUG=chromium-os:10138
TEST=Run chrome os with a color profile, check that it gets loaded (I use a profile with one of the components zeroed for testing so changes are obvious).
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=372eb64
Patch Set 1 #
Total comments: 22
Patch Set 2 : Fix style. #Patch Set 3 : Add a command line option for gamma. #
Total comments: 1
Patch Set 4 : git cl push #
Messages
Total messages: 12 (0 generated)
This does nothing ATM (it won't touch the gamma ramp if it doesn't find the color profile). Trying to land this by R12...
Looking at it now. On Mon, Apr 18, 2011 at 4:46 PM, <marcheu@chromium.org> wrote: > Reviewers: Daniel Erat, > > Message: > This does nothing ATM (it won't touch the gamma ramp if it doesn't find the > color profile). Trying to land this by R12... > > Description: > Add gamma support to ply-image. > > BUG=none > TEST= > > > Please review this at http://codereview.chromium.org/6877024/ > > SVN Base: ssh://gitrw.chromium.org:9222/ply-image.git@master > > Affected files: > M src/Makefile > A src/ply-frame-buffer.c > A src/ply-gamma.h > A src/ply-gamma.c > M src/ply-image.c > > >
LGTM assuming that ply-gamma.c was taken "as is" from the ply-image distro. (Perhaps the review should mention this, and which version.) Also, did something go wrong with the upload of ply-frame-buffer.c? (+-1 lines, --1 lines). Let me know if there is something in there that needs reviewing. In addition, consider that if the gamma translation slows down the rendering, we may reduce the (already short) time available for the animation, so that (future) review should contain before/after numbers. Finally, other reviewers make me create a bug if one doesn't exist, then add chromium-os:12345 to the bug: field. I know how much fun that is, so I'll leave that up to you. Thanks! On 2011/04/19 00:40:31, Luigi Semenzato wrote: > Looking at it now. > > On Mon, Apr 18, 2011 at 4:46 PM, <mailto:marcheu@chromium.org> wrote: > > Reviewers: Daniel Erat, > > > > Message: > > This does nothing ATM (it won't touch the gamma ramp if it doesn't find the > > color profile). Trying to land this by R12... > > > > Description: > > Add gamma support to ply-image. > > > > BUG=none > > TEST= > > > > > > Please review this at http://codereview.chromium.org/6877024/ > > > > SVN Base: ssh://gitrw.chromium.org:9222/ply-image.git@master > > > > Affected files: > > M src/Makefile > > A src/ply-frame-buffer.c > > A src/ply-gamma.h > > A src/ply-gamma.c > > M src/ply-image.c > > > > > >
On 2011/04/19 00:57:26, Luigi Semenzato wrote: > LGTM assuming that ply-gamma.c was taken "as is" from the ply-image distro. > (Perhaps the review should mention this, and which version.) > Nope, it wasn't taken as-is; I wrote the code myself. Plymouth doesn't have support for gamma so I had to actually write it. > Also, did something go wrong with the upload of ply-frame-buffer.c? (+-1 lines, > --1 lines). Let me know if there is something in there that needs reviewing. It does this because I removed the unix "execute bit" from the .c file. > > In addition, consider that if the gamma translation slows down the rendering, we > may reduce the (already short) time available for the animation, so that > (future) review should contain before/after numbers. > According to printf statements the whole gamma function takes less than a millisecond to execute. > Finally, other reviewers make me create a bug if one doesn't exist, then add > chromium-os:12345 to the bug: field. I know how much fun that is, so I'll leave > that up to you. > Right, actually there's already a bug I'll fix that.
LGTM after fixing a bunch of style things. http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c File src/ply-gamma.c (right): http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode27 src/ply-gamma.c:27: #include "ply-gamma.h" nit: this include should come first http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode30 src/ply-gamma.c:30: static const int internalPanel = 1; name variables like_this instead of likeThis http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode36 src/ply-gamma.c:36: } gamma_ramp; this should be GammaRamp instead (at least, i think it makes sense to stay as close to the C++ style guide as you can in C code) http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode39 src/ply-gamma.c:39: { move to end of previous line here and elsewhere http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode40 src/ply-gamma.c:40: const int gamma_size = 256; kGammaSize http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode49 src/ply-gamma.c:49: r += fread(red, gamma_size, 1, f); can you s/gamma_size/sizeof(red)/ here? http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode63 src/ply-gamma.c:63: { you know what i'm going to say here http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode81 src/ply-gamma.c:81: ply_gamma_set() return type and function name should be on same line http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode87 src/ply-gamma.c:87: ramp = load_gamma_ramp (gammaRampFile); no space between function name and opening parenthesis here and elsewhere http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode123 src/ply-gamma.c:123: remove extra newlines at end of file http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.h File src/ply-gamma.h (right): http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.h#newcode26 src/ply-gamma.h:26: #include <stdint.h> nit: do you really need stdint.h here?
Actually, aren't we supposed to use the existing style when adding code to a program? This is a new, separate file, but most of the original code is still taken as is from ply-image. On Tue, Apr 19, 2011 at 9:46 AM, <derat@chromium.org> wrote: > LGTM after fixing a bunch of style things. > > > http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c > File src/ply-gamma.c (right): > > http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode27 > src/ply-gamma.c:27: #include "ply-gamma.h" > nit: this include should come first > > http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode30 > src/ply-gamma.c:30: static const int internalPanel = 1; > name variables like_this instead of likeThis > > http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode36 > src/ply-gamma.c:36: } gamma_ramp; > this should be GammaRamp instead (at least, i think it makes sense to > stay as close to the C++ style guide as you can in C code) > > http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode39 > src/ply-gamma.c:39: { > move to end of previous line here and elsewhere > > http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode40 > src/ply-gamma.c:40: const int gamma_size = 256; > kGammaSize > > http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode49 > src/ply-gamma.c:49: r += fread(red, gamma_size, 1, f); > can you s/gamma_size/sizeof(red)/ here? > > http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode63 > src/ply-gamma.c:63: { > you know what i'm going to say here > > http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode81 > src/ply-gamma.c:81: ply_gamma_set() > return type and function name should be on same line > > http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode87 > src/ply-gamma.c:87: ramp = load_gamma_ramp (gammaRampFile); > no space between function name and opening parenthesis here and > elsewhere > > http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode123 > src/ply-gamma.c:123: > remove extra newlines at end of file > > http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.h > File src/ply-gamma.h (right): > > http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.h#newcode26 > src/ply-gamma.h:26: #include <stdint.h> > nit: do you really need stdint.h here? > > http://codereview.chromium.org/6877024/ >
Stephane asked me about this. My take is that the style in the existing files is so ugly and internally inconsistent ('{' on same line for function declarations but on a new line and indented two spaces for 'if'?) that we're better off going with something sane for new files (and ideally, cleaning up the existing code as well). On Tue, Apr 19, 2011 at 10:30, Luigi Semenzato <semenzato@chromium.org> wrote: > Actually, aren't we supposed to use the existing style when adding > code to a program? This is a new, separate file, but most of the > original code is still taken as is from ply-image. > > On Tue, Apr 19, 2011 at 9:46 AM, <derat@chromium.org> wrote: >> LGTM after fixing a bunch of style things. >> >> >> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c >> File src/ply-gamma.c (right): >> >> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode27 >> src/ply-gamma.c:27: #include "ply-gamma.h" >> nit: this include should come first >> >> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode30 >> src/ply-gamma.c:30: static const int internalPanel = 1; >> name variables like_this instead of likeThis >> >> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode36 >> src/ply-gamma.c:36: } gamma_ramp; >> this should be GammaRamp instead (at least, i think it makes sense to >> stay as close to the C++ style guide as you can in C code) >> >> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode39 >> src/ply-gamma.c:39: { >> move to end of previous line here and elsewhere >> >> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode40 >> src/ply-gamma.c:40: const int gamma_size = 256; >> kGammaSize >> >> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode49 >> src/ply-gamma.c:49: r += fread(red, gamma_size, 1, f); >> can you s/gamma_size/sizeof(red)/ here? >> >> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode63 >> src/ply-gamma.c:63: { >> you know what i'm going to say here >> >> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode81 >> src/ply-gamma.c:81: ply_gamma_set() >> return type and function name should be on same line >> >> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode87 >> src/ply-gamma.c:87: ramp = load_gamma_ramp (gammaRampFile); >> no space between function name and opening parenthesis here and >> elsewhere >> >> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode123 >> src/ply-gamma.c:123: >> remove extra newlines at end of file >> >> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.h >> File src/ply-gamma.h (right): >> >> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.h#newcode26 >> src/ply-gamma.h:26: #include <stdint.h> >> nit: do you really need stdint.h here? >> >> http://codereview.chromium.org/6877024/ >> >
OK, it makes sense. It's not like we're ever merging with that code again. On Tue, Apr 19, 2011 at 10:35 AM, Daniel Erat <derat@chromium.org> wrote: > Stephane asked me about this. My take is that the style in the > existing files is so ugly and internally inconsistent ('{' on same > line for function declarations but on a new line and indented two > spaces for 'if'?) that we're better off going with something sane for > new files (and ideally, cleaning up the existing code as well). > > On Tue, Apr 19, 2011 at 10:30, Luigi Semenzato <semenzato@chromium.org> wrote: >> Actually, aren't we supposed to use the existing style when adding >> code to a program? This is a new, separate file, but most of the >> original code is still taken as is from ply-image. >> >> On Tue, Apr 19, 2011 at 9:46 AM, <derat@chromium.org> wrote: >>> LGTM after fixing a bunch of style things. >>> >>> >>> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c >>> File src/ply-gamma.c (right): >>> >>> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode27 >>> src/ply-gamma.c:27: #include "ply-gamma.h" >>> nit: this include should come first >>> >>> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode30 >>> src/ply-gamma.c:30: static const int internalPanel = 1; >>> name variables like_this instead of likeThis >>> >>> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode36 >>> src/ply-gamma.c:36: } gamma_ramp; >>> this should be GammaRamp instead (at least, i think it makes sense to >>> stay as close to the C++ style guide as you can in C code) >>> >>> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode39 >>> src/ply-gamma.c:39: { >>> move to end of previous line here and elsewhere >>> >>> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode40 >>> src/ply-gamma.c:40: const int gamma_size = 256; >>> kGammaSize >>> >>> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode49 >>> src/ply-gamma.c:49: r += fread(red, gamma_size, 1, f); >>> can you s/gamma_size/sizeof(red)/ here? >>> >>> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode63 >>> src/ply-gamma.c:63: { >>> you know what i'm going to say here >>> >>> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode81 >>> src/ply-gamma.c:81: ply_gamma_set() >>> return type and function name should be on same line >>> >>> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode87 >>> src/ply-gamma.c:87: ramp = load_gamma_ramp (gammaRampFile); >>> no space between function name and opening parenthesis here and >>> elsewhere >>> >>> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode123 >>> src/ply-gamma.c:123: >>> remove extra newlines at end of file >>> >>> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.h >>> File src/ply-gamma.h (right): >>> >>> http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.h#newcode26 >>> src/ply-gamma.h:26: #include <stdint.h> >>> nit: do you really need stdint.h here? >>> >>> http://codereview.chromium.org/6877024/ >>> >> >
http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c File src/ply-gamma.c (right): http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode27 src/ply-gamma.c:27: #include "ply-gamma.h" On 2011/04/19 16:46:02, Daniel Erat wrote: > nit: this include should come first Done. http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode30 src/ply-gamma.c:30: static const int internalPanel = 1; On 2011/04/19 16:46:02, Daniel Erat wrote: > name variables like_this instead of likeThis Done but differently per our discussion. http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode36 src/ply-gamma.c:36: } gamma_ramp; On 2011/04/19 16:46:02, Daniel Erat wrote: > this should be GammaRamp instead (at least, i think it makes sense to stay as > close to the C++ style guide as you can in C code) Done. http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode39 src/ply-gamma.c:39: { On 2011/04/19 16:46:02, Daniel Erat wrote: > move to end of previous line here and elsewhere Done. http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode40 src/ply-gamma.c:40: const int gamma_size = 256; On 2011/04/19 16:46:02, Daniel Erat wrote: > kGammaSize Done. http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode49 src/ply-gamma.c:49: r += fread(red, gamma_size, 1, f); On 2011/04/19 16:46:02, Daniel Erat wrote: > can you s/gamma_size/sizeof(red)/ here? Done. http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode63 src/ply-gamma.c:63: { On 2011/04/19 16:46:02, Daniel Erat wrote: > you know what i'm going to say here Done. http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode81 src/ply-gamma.c:81: ply_gamma_set() On 2011/04/19 16:46:02, Daniel Erat wrote: > return type and function name should be on same line Done. http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode87 src/ply-gamma.c:87: ramp = load_gamma_ramp (gammaRampFile); On 2011/04/19 16:46:02, Daniel Erat wrote: > no space between function name and opening parenthesis here and elsewhere Done. http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.c#newcode123 src/ply-gamma.c:123: On 2011/04/19 16:46:02, Daniel Erat wrote: > remove extra newlines at end of file Done. http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.h File src/ply-gamma.h (right): http://codereview.chromium.org/6877024/diff/1/src/ply-gamma.h#newcode26 src/ply-gamma.h:26: #include <stdint.h> On 2011/04/19 16:46:02, Daniel Erat wrote: > nit: do you really need stdint.h here? Moved it to .c file.
Added gamma command line option, PTAL.
LooksDisgustingToMe, but not your fault. Ship it!
http://codereview.chromium.org/6877024/diff/2005/src/ply-image.c File src/ply-image.c (right): http://codereview.chromium.org/6877024/diff/2005/src/ply-image.c#newcode521 src/ply-image.c:521: " ply-image <background> [<x-offset> <y-offset> " forgot to mention: add something about --gamma here |