GNOME Bugzilla – Bug 789642
operations/common/saturation: Support consuming data from operations not setting an output format in "prepare"
Last modified: 2017-10-30 12:19:51 UTC
It is ready to accept any input format (eg., CIE Lab, sRGB, etc.) and claims to be able to output linear RGB with premultiplied alpha. That's wrong.
Created attachment 362521 [details] [review] operations/common/gblur-1d: Fix the input and output Babl formats
(In reply to Debarshi Ray from comment #0) > It is ready to accept any input format (eg., CIE Lab, sRGB, etc.) and claims > to be able to output linear RGB with premultiplied alpha. That's wrong. I don't understand what's wrong. Do you have a failing test case? The input format is not used, the input buffer is converted to the output format when it is read. OTOH with the attached patch gegl crashes when the input operation does not set the output format, for instance: > $ gegl -x '<gegl><gegl:crop width="128" height="128"/><gegl:color/></gegl>' -o tmp.gegl > $ gegl -x '<gegl><gegl:gblur-1d/><gegl:load path="tmp.gegl"/></gegl>' -o tmp.ppm
(In reply to Massimo from comment #2) > (In reply to Debarshi Ray from comment #0) > > It is ready to accept any input format (eg., CIE Lab, sRGB, etc.) and claims > > to be able to output linear RGB with premultiplied alpha. That's wrong. > > I don't understand what's wrong. Do you have a failing test case? No. I was reading the code to understand what Babl conversions are being used by gegl:shadows-highlights so that I could speed things up. The lack of an input format surprised me. I missed the fact that the input format is being implicitly enforced while reading the source buffer. Sorry about that. > The input format is not used, the input buffer is converted to the > output format when it is read. Could we still explicitly specify the input format in prepare, even though it's not necessary? It might make the code marginally easier to read. > OTOH with the attached patch gegl crashes when the input operation > does not set the output format, for instance: > > > $ gegl -x '<gegl><gegl:crop width="128" height="128"/><gegl:color/></gegl>' -o tmp.gegl > > $ gegl -x '<gegl><gegl:gblur-1d/><gegl:load path="tmp.gegl"/></gegl>' -o tmp.ppm Interesting. I didn't realize that many of the sources never specify their output formats during "prepare". This means that at least gegl:saturation needs to be fixed. Maybe we should update the test suite to cover this scenario?
Created attachment 362536 [details] [review] operations/common/saturation: Always set input & output formats
Created attachment 362537 [details] [review] operations/common/gblur-1d: Explicitly set the input format
(In reply to Debarshi Ray from comment #3) > (In reply to Massimo from comment #2) ... > > > The input format is not used, the input buffer is converted to the > > output format when it is read. > > Could we still explicitly specify the input format in prepare, even though > it's not necessary? It might make the code marginally easier to read. > I find it confusing when it is not used (directly or in the base), but I learned to ignore it in these cases. So for me yes. > > OTOH with the attached patch gegl crashes when the input operation > > does not set the output format, for instance: > > > > > $ gegl -x '<gegl><gegl:crop width="128" height="128"/><gegl:color/></gegl>' -o tmp.gegl > > > $ gegl -x '<gegl><gegl:gblur-1d/><gegl:load path="tmp.gegl"/></gegl>' -o tmp.ppm > > Interesting. I didn't realize that many of the sources never specify their > output formats during "prepare". This means that at least gegl:saturation > needs to be fixed. Maybe we should update the test suite to cover this > scenario? Test suite or not, operations are better be coded to deal with the case where the output format is not set.
Ok, thanks Massimo. I pushed the patches to master. Sorry for the confusion.