I replied already on the ticket, but now I've thought more about the issues and
so I'll follow up here.
What should we do for formats that do not support differing 'data'
(width,height) and 'display' windows (full_width, full_height)? And similarly,
for formats that don't support nonzero origins (x,y,full_x,full_y)? There are
two possible approaches:
1. Write the original width x height pixels, but just omit the
full_width,full_height metadata.
2. Write an image of full_width x full_height, padding with black pixels.
I suppose it depends on philosophy of what the windows "mean". I think of
width,height (aka data window) as the *actual* image resolution, and
full_width,full_height (and x,y,full_x,full_y) as additional, optional
information that says "here's where I think those pixels should be placed,
possibly in a differently-shaped virtual canvas or displayable window". The
alternate approach, I suppose, is to consider the display window to be the
"real" image, and the x,y,width,height to be extra information saying "as a
shortcut, I'm only going to give you a subset of those pixels, or alternately,
extra pixels extending beyond the border." Both are equally valid and true,
but we may differ in which we think gets at the core relationships.
I'm inclined to vote for #1, both because of my philosophical leaning above,
and on the grounds that the best policy is "write the pixels the app hands you,
and change as little else as possible." Changing outside-display-window pixels
from "undefined" to "black" is a big leap in my book, as would be dropping
overscan pixels, which I think are the things that happen in Hugh's patch. I
don't think that's something an ImageOutput ought to decide to do on its own.
In fact, I claim that all the ImageOutput implementations already have the best
low-level behavior -- they will write the actual pixels passed from the app,
and save the metadata about the origin and "full" (display) window if the
output format supports it (otherwise silently dropping it). So I'm leaning
toward believing that none of the output plugins need to change at all (except
for possibly adding new supports() tags so they can communicate their support
for origins and/or full windows, that's definitely a handy idea).
I'm willing to believe that a behavior change at the ImageBuf or oiiotool level
is appropriate. Is ImageBuf really causes a problem in this case? I'm not sure
I'm convinced yet.
Really, I think that what Hugh wants is to beef up the behavior of oiiotool to
more gracefully handle operations on multiple images that do not all share a
common resolution and offset. In fact, it may also be awkward for non-zero
offsets and mismatched display and data windows, even if all images match each
other. Let's fix that, I think it more directly gets us where we want to go.
As part of this, I think it's perfectly reasonable to have oiiotool options
that can choose among different behaviors when there are mismatches, or when
outputs are directed toward files with formats that don't support virtual
canvases ("pad black pixels" may be a perfectly reasonable choice among those
options).
Sorry, Hugh, believe it or not, I did not wake up this morning with the goal of
making all of your pull requests moot! In fact, I really liked this patch when
I first saw it, was ready to give it a "LGTM", but the more I think about it,
the more I believe it's attacking the problem at the wrong level of abstraction.
-- lg
On Oct 3, 2011, at 6:34 AM, Hugh Macdonald wrote:
> We discovered last week that, if we took an EXR with a data window that was
> not it's display window (let's say, for example, that the display window is
> 2048x1556, but the data window has the soundtrack area cropped off, and is
> 1828x1556, with an xoffset of 220), if you used oiiotool to convert this EXR
> to a DPX, the DPX would end up the size of the data window and not the size
> of the display window.
>
> I've submitted a pull request (Issue #176) for a fix for this. It works as
> follows:
>
> ImageOutput::supports() now has an extra feature that it may expect -
> 'datawindow'. Currently, only EXR, RLA and TIF return true for this.
>
> In the open() function for each of ImageOutput's subclasses, instead of
> setting 'm_spec = userspec;', they now call 'stash_spec(userspec);'. This
> sets m_spec, but also, if supports('datawindow') returns false, sets
> m_spec.width = m_spec.full_width, m_spec.height = m_spec.full_height, etc.
>
> Lastly, in ImageBuf::write(), if the ImageOutput that it's going to write to
> does not support('datawindow'), and the ImageBuf's width/height/etc don't
> match full_width/full_height/etc, then it calls ImageBufAlgo::crop() to
> create a new ImageBuf object with the right amount of data in, and then calls
> write() on the new ImageBuf object.
>
>
> Any thoughts? I'm thinking that the method for padding/cropping could be more
> efficient, but I didn't want to be changing the ImageBuf in place, just in
> case it had wider-ranging effects.
>
> Hugh Macdonald
> nvizible – VISUAL EFFECTS
>
> [email protected]
> +44(0) 20 3167 3860
> +44(0) 7773 764 708
>
> www.nvizible.com
>
> _______________________________________________
> Oiio-dev mailing list
> [email protected]
> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
--
Larry Gritz
[email protected]
_______________________________________________
Oiio-dev mailing list
[email protected]
http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
