While working on a partial image reading patch, I came accross the 
following bug in POVRay: 

In jpeg_pov.cpp, around line 405, the code reads: 

-------------------<jpeg_pov.cpp:405>---------------------------------
        bufptr->row_stride = w * 3;

        bufptr->row_pointer[0] = (JSAMPROW)POV_MALLOC(
                bufptr->row_stride * w, "JPEG line buffer");
----------------------------------------------------------------------

And some lines below: 

-------------------<jpeg_pov.cpp:636>---------------------------------
        /* JSAMPLEs per row in output buffer */
        bufptr.row_stride = 
                bufptr.cinfo.output_width * bufptr.cinfo.output_components;
        /* Make a one-row-high sample array */
        bufptr.row_pointer[0] = (JSAMPROW)POV_MALLOC(
                bufptr.row_stride * width, "JPEG line buffer");
----------------------------------------------------------------------

Both chunks of code contain the same stupid bug which causes excessive 
memory consumption: 

row_stride is the amount of memory in bytes needed for one ROW in the 
image. What is being done here is allocating this amount of memory for 
EVERY ROW in the image. 

This means that for bpp bytes per pixel, we end up allocating 
   width*width*bpp bytes 
where only 
   width*bpp bytes
are needed! [In my case that meant 768Mb instead of 50kb.]

Please apply the following patch: 

-------------------<jpeg_pov.cpp:405>---------------------------------
        bufptr->row_stride = w * 3;

        bufptr->row_pointer[0] = (JSAMPROW)POV_MALLOC(
                bufptr->row_stride, "JPEG line buffer");
----------------------------------------------------------------------

-------------------<jpeg_pov.cpp:636>---------------------------------
        /* JSAMPLEs per row in output buffer */
        bufptr.row_stride = 
                bufptr.cinfo.output_width * bufptr.cinfo.output_components;
        /* Make a one-row-high sample array */
        bufptr.row_pointer[0] = (JSAMPROW)POV_MALLOC(
                bufptr.row_stride, "JPEG line buffer");
----------------------------------------------------------------------

This modification (effectively removing the with multiplication in the 
memory allocation size calculation) has been verified to work correctly 
by me. Note that row_pointer[0] is always only meant to contain one 
single image row, never more. 

Regards,
Wolfgang

I hope I can have a look at POVRay's next beta version (linux source 
code) real soon.

Post a reply to this message

Wolfgang Wieser wrote:
> While working on a partial image reading patch, I came accross the 
> following bug in POVRay: 

You'd probably save some time if you wait until 3.6 is out because 
otherwise you would need to rewrite the code (image reading/writing has 
not changed much but some changes require modifications at a lot of 
places in the code).

> In jpeg_pov.cpp, around line 405, the code reads: 
> 
> [...]

Sounds logical to me although i have no experience with libjpeg.

Just in case this is not clear to everyone - this is not a memory leak, 
it is just POV-Ray temporarily allocates more memory than it needs.

> 
> I hope I can have a look at POVRay's next beta version (linux source 
> code) real soon. 

As usual source will not be available during beta.

Christoph

-- 
POV-Ray tutorials, include files, Sim-POV,
HCR-Edit and more: http://www.tu-bs.de/~y0013390/
Last updated 11 Jan. 2004 _____./\/^>_*_<^\/\.______

Post a reply to this message

Christoph Hormann wrote:

> Wolfgang Wieser wrote:
>> While working on a partial image reading patch, I came accross the
>> following bug in POVRay:
> 
> You'd probably save some time if you wait until 3.6 is out because
> otherwise you would need to rewrite the code (image reading/writing has
> not changed much but some changes require modifications at a lot of
> places in the code).
> 
Well, I really see your point but I am actually very tired of _waiting_. 
If anybody out there is waiting for the POVRay team to do just the smallest 
thing, he'd better invest in finding the fountain of youth ;)

BTW, what about including this patch into POVRay-3.6? 
Basically, the patch allows you to select a rectangular region of 
an image_map so that only this part is read in and stored in RAM. 
Pixels outside this region are all considered 50% gray (some other 
solution like quick_color could also be thought of). 

Of course, inclusion only makes sense if there are more people 
interested in it. I am currently using it to render MARS images, 
because I cannot fit all the 2.4 Gb topo & color into RAM. 

> Just in case this is not clear to everyone - this is not a memory leak,
> it is just POV-Ray temporarily allocates more memory than it needs.
> 
Correct. 

>> I hope I can have a look at POVRay's next beta version (linux source
>> code) real soon.
> 
> As usual source will not be available during beta.
> 
It may or may not make sense to publish source code of beta versions. 
But as I am sombody who actually works with the code, has already found 
several bugs, suggested fixes for most of them and is actively writing 
patches for POVRay, it may make sense to let me join the group of people 
who may view the current development code. 

Regards,
Wolfgang

Post a reply to this message

Wolfgang Wieser wrote:
> 
> Well, I really see your point but I am actually very tired of _waiting_. 
> If anybody out there is waiting for the POVRay team to do just the smallest 
> thing, he'd better invest in finding the fountain of youth ;)

Well i only suggested you might *save* time by waiting.

> BTW, what about including this patch into POVRay-3.6? 

As Chris Cason has mentioned in the status report 3.6 does not introduce 
significant new features in the core functionality.

> Basically, the patch allows you to select a rectangular region of 
> an image_map so that only this part is read in and stored in RAM. 
> Pixels outside this region are all considered 50% gray (some other 
> solution like quick_color could also be thought of). 

I don't think this would be a very useful patch - selecting and copying 
a rectangle from a larger image is the task for an external program, 
what would be really useful would be a patch that adaptively loads those 
parts on an image into memory that are actually used - without the need 
to specify a rectangle and that also unloads those parts that are no 
more needed during render progress to free space for new parts.  Of 
course such a feature would not only be useful for images but also for 
meshes for example.  AFAIK some other raytracing programs also have such 
a feature.

> It may or may not make sense to publish source code of beta versions. 
> But as I am sombody who actually works with the code, has already found 
> several bugs, suggested fixes for most of them and is actively writing 
> patches for POVRay, it may make sense to let me join the group of people 
> who may view the current development code. 

This is of course no way my decision but to be frank - it does not seem 
to me that you propose to be allowed source access primarily to fix 
bugs, this is perfectly possible with the 3.5 code and some of your 
proposals have already been included in 3.6 (and the last two probably 
will be as well).

Christoph

-- 
POV-Ray tutorials, include files, Sim-POV,
HCR-Edit and more: http://www.tu-bs.de/~y0013390/
Last updated 11 Jan. 2004 _____./\/^>_*_<^\/\.______

Post a reply to this message

In article <401291b0@news.povray.org>, Wolfgang Wieser <wwi### [at] gmxde> 
wrote:

> BTW, what about including this patch into POVRay-3.6? 

It's too late for that. Public betas are already starting, the only 
things that will go into POV from now until the 3.6 final release are 
bug fixes. The over-allocation fix might get in, but no new features or 
major changes.


> Basically, the patch allows you to select a rectangular region of 
> an image_map so that only this part is read in and stored in RAM. 
> Pixels outside this region are all considered 50% gray (some other 
> solution like quick_color could also be thought of). 

Pixels outside that region should probably be fully transparent, to be 
consistent with the "once" option. And why not read in the region as the 
entire image, filling the entire area of the image_map? Simpler, and 
avoids the above problem completely.

-- 
Christopher James Huff <cja### [at] earthlinknet>
http://home.earthlink.net/~cjameshuff/
POV-Ray TAG: <chr### [at] tagpovrayorg>
http://tag.povray.org/

Post a reply to this message

Christoph Hormann wrote:

> As Chris Cason has mentioned in the status report 3.6 does not introduce
> significant new features in the core functionality.
> 
...which is perfectly acceptable (to me). 

> I don't think this would be a very useful patch - selecting and copying
> a rectangle from a larger image is the task for an external program,
>
Yes, but there are two reasons for my approach: 
(1) Opening a 1Gb file in an external program, selecting some part, 
    saving it... all that for every view port is too much work for me. 
(2) The cut-down chunk needs extra calculations to make sure the 
    image map projection is exactly correct. 
My patch saves me from both these. 

Still it is probably a very special thingy and of little use for 
most people. 

> what would be really useful would be a patch that adaptively loads those
> parts on an image into memory that are actually used - without the need
> to specify a rectangle and that also unloads those parts that are no
> more needed during render progress to free space for new parts.  Of
> course such a feature would not only be useful for images but also for
> meshes for example.  AFAIK some other raytracing programs also have such
> a feature.
> 
I actually had the plan to implement something like that for meshes 
before I came accross the possibility using the isosurface for the topo. 

In-memory (de)compression of the data was another approach I thought of. 

> This is of course no way my decision but to be frank - it does not seem
> to me that you propose to be allowed source access primarily to fix
> bugs, this is perfectly possible with the 3.5 code and some of your
> proposals have already been included in 3.6 (and the last two probably
> will be as well).
> 
But...
> Well i only suggested you might *save* time by waiting.

I was told the same argument some time ago. It read like "we did a 
complete rewrite of unix.cpp but you can do everything with the old 
version so feel free to send us patches for the old code". 

I could consider that as an expression of no interest in what other 
people are doing. It's a bit like preaching water and drinking wine. 

Don't you think that this effectively hinders futher development?
It makes me waste time writing patches for old code which have to 
be converted to the new code. It makes the POVRay team waste time 
getting patches based on one-year old code base. And finally, most 
of the bugs I see come from looking at the source, not from using 
a binary. 

Don't understand me wrong: I respect any decision the POVRay team 
makes. 

But I see a further possibility using the following roadmap:
(1) get povray 3.6 out
(2) bring out long awaited new megapov based on 3.6
(3) more frequent megapov updates

In this case I'd be more happy to see current megapov development 
and to be able to write patches for (or even contribute to) megapov 
[than to mainline povray] anyways. 

To me (as looking at it from outside) it seems like there would be 
enough ideas, code and manpower around here for quite some innovation 
but there is somebody somewhere out there actively pulling the break. 

I learned that in most cases it turns out negatively to hinder those 
people who want to work (and have the ability to do it well) from 
doing their work. 

Wolfgang

Post a reply to this message

In article <401291b0@news.povray.org> , Wolfgang Wieser <wwi### [at] gmxde>  
wrote:

> Well, I really see your point but I am actually very tired of _waiting_.

Well, then  start your own ray-tracer project.  Oh, well, wait, every single
one ever started has died about as quickly.  Quality software development
takes time, otherwise you would end up with bloated junk and broken 1.0
designs!  For example Mozilla is a great example for a design broken from
the beginning...

    Thorsten

____________________________________________________
Thorsten Froehlich, Duisburg, Germany
e-mail: tho### [at] trfde

Visit POV-Ray on the web: http://mac.povray.org

Post a reply to this message

In article <401291b0@news.povray.org> , Wolfgang Wieser <wwi### [at] gmxde>  
wrote:

> BTW, what about including this patch into POVRay-3.6?

This ain't a patch, this is a trivial bug fix!

    Thorsten

____________________________________________________
Thorsten Froehlich, Duisburg, Germany
e-mail: tho### [at] trfde

Visit POV-Ray on the web: http://mac.povray.org

Post a reply to this message

Christopher James Huff wrote:

>> Basically, the patch allows you to select a rectangular region of
>> an image_map so that only this part is read in and stored in RAM.
>> Pixels outside this region are all considered 50% gray (some other
>> solution like quick_color could also be thought of).
> 
> Pixels outside that region should probably be fully transparent, to be
> consistent with the "once" option. 
>
I would prefer the pixels to be of configurable color...

> And why not read in the region as the
> entire image, filling the entire area of the image_map? Simpler, and
> avoids the above problem completely.
> 
I do not completely understand what you plan there but it seems to me 
that this requires the used to do additional scaling and 
rotation/translation of the texture (which may not be trivial for 
some projections). 
Furthermore, for my current personal use, the 50% gray is a "feature" 
(sea level on Mars). 

Wolfgang

Post a reply to this message

Thorsten Froehlich wrote:
>> BTW, what about including this patch into POVRay-3.6?
> 
> This ain't a patch, this is a trivial bug fix!
> 
I was talking about the partial image reading. 

Wolfgang

Post a reply to this message