From: Brendan M. <bre...@gm...> - 2007-12-20 01:44:38
Attachments:
vil_jpg_diffs.txt
vgl_diffs.txt
|
G'day All, I've incorporated some changes to core/vil/file_formats/vil_j2k_image.* core/vil/file_formats/vil_jpeg_compressor.* largely to do with calls to get_view_decimated, and adding a set_quality method. This has some potential to cause conflicts with existing code that relies on the promotion of unsigned to double, but otherwise looks cleaner I think. Also vgl_polygon_scan_iterator has been refactored to an iterative version, as the current recursive version causes stack overflow on very large polygons. The diffs are included below. Can anyone see any issues with these changes? Shall I go ahead and check them in? -- Cheers, Brendan |
From: Brendan M. <bre...@gm...> - 2007-12-23 07:25:26
|
My last message doesn't seem to have got through, here it is again (I've inlined the attachments in case that was the problem) ... G'day All, I've incorporated some changes to core/vil/file_formats/vil_j2k_image.* core/vil/file_formats/vil_jpeg_compressor.* largely to do with calls to get_view_decimated, and adding a set_quality method. This has some potential to cause conflicts with existing code that relies on the promotion of unsigned to double, but otherwise looks cleaner I think. Also vgl_polygon_scan_iterator has been refactored to an iterative version, as the current recursive version causes stack overflow on very large polygons. The diffs are included below. Can anyone see any issues with these changes? Shall I go ahead and check them in? -- Cheers, Brendan ? vil_jpg_diffs.txt Index: vil_j2k_image.cxx =================================================================== RCS file: /cvsroot/vxl/vxl/core/vil/file_formats/vil_j2k_image.cxx,v retrieving revision 1.2 diff -r1.2 vil_j2k_image.cxx 169a170,184 > return get_copy_view_decimated(sample0, > num_samples, > line0, > numLines, > (unsigned int) ( ((double)num_samples)/i_factor ), > (unsigned int) ( ((double)numLines)/j_factor )); > } > > vil_image_view_base_sptr vil_j2k_image::get_copy_view_decimated(unsigned sample0, > unsigned num_samples, > unsigned line0, > unsigned numLines, > unsigned int output_width, > unsigned int output_height) const > { 187,188d201 < unsigned int output_width = (unsigned int) ( ((double)num_samples)/i_factor ); < unsigned int output_height = (unsigned int) ( ((double)numLines)/j_factor ); 262a276 > // </LG> 269c283 < return get_copy_view_decimated( sample0, num_samples, line0, numLines, 1.0, 1.0 ); --- > return get_copy_view_decimated( sample0, num_samples, line0, numLines, num_samples, numLines ); 296a311,321 > > vil_image_view_base_sptr vil_j2k_image::s_decode_jpeg_2000( vil_stream* vs, > unsigned i0, unsigned ni, > unsigned j0, unsigned nj, > unsigned int output_width, unsigned int output_height ) > { > vil_j2k_image* j2k_image = new vil_j2k_image(vs); > vil_image_view_base_sptr view = j2k_image->get_copy_view_decimated(i0, ni, j0, nj, output_width, output_height); > delete j2k_image; > return view; > } Index: vil_j2k_image.h =================================================================== RCS file: /cvsroot/vxl/vxl/core/vil/file_formats/vil_j2k_image.h,v retrieving revision 1.2 diff -r1.2 vil_j2k_image.h 92a93,96 > virtual vil_image_view_base_sptr get_copy_view_decimated(unsigned i0, unsigned ni, > unsigned j0, unsigned nj, > unsigned int output_width, unsigned int output_height) const; > 121a126,130 > static vil_image_view_base_sptr s_decode_jpeg_2000( vil_stream* vs, > unsigned i0, unsigned ni, > unsigned j0, unsigned nj, > unsigned int output_width, unsigned int output_height ); > Index: vil_jpeg_compressor.cxx =================================================================== RCS file: /cvsroot/vxl/vxl/core/vil/file_formats/vil_jpeg_compressor.cxx,v retrieving revision 1.7 diff -r1.7 vil_jpeg_compressor.cxx 17a18,21 > // this is the value assigned during jpeg_set_defaults(). > // It would be nice to not use a magic number here. > int vil_jpeg_compressor::quality = 75; > 59a64 > jpeg_set_quality(&jobj, quality, TRUE); 101a107,115 > void vil_jpeg_compressor::set_quality(int q) > { > quality = q; > } > > int vil_jpeg_compressor::get_quality() > { > return quality; > } Index: vil_jpeg_compressor.h =================================================================== RCS file: /cvsroot/vxl/vxl/core/vil/file_formats/vil_jpeg_compressor.h,v retrieving revision 1.6 diff -r1.6 vil_jpeg_compressor.h 31a32,34 > static void set_quality(int quality); > static int get_quality(); > 33a37 > static int quality; ? vgl_diffs.txt Index: vgl_polygon_scan_iterator.txx =================================================================== RCS file: /cvsroot/vxl/vxl/core/vgl/vgl_polygon_scan_iterator.txx,v retrieving revision 1.5 diff -r1.5 vgl_polygon_scan_iterator.txx 291,303c291,293 < // Find next segment on current scan line < if ( curcrossedge < numcrossedges ) < { < fxl = crossedges[curcrossedge].x; < fxr = crossedges[curcrossedge+1].x; < if (boundp) < // left end of span with boundary < xl = (int)vcl_floor( crossedges[curcrossedge].x - vgl_polygon_scan_iterator_offset); < else < // left end of span without boundary < xl = (int)vcl_ceil( crossedges[curcrossedge].x - vgl_polygon_scan_iterator_offset); < < if ( have_window && xl < irnd(win.min_x()) ) --- > do { > // Find next segment on current scan line > while ( curcrossedge < numcrossedges ) 305,307c295,302 < fxl = win.min_x(); < xl = irnd(fxl); < } --- > fxl = crossedges[curcrossedge].x; > fxr = crossedges[curcrossedge+1].x; > if (boundp) > // left end of span with boundary > xl = (int)vcl_floor( crossedges[curcrossedge].x - vgl_polygon_scan_iterator_offset); > else > // left end of span without boundary > xl = (int)vcl_ceil( crossedges[curcrossedge].x - vgl_polygon_scan_iterator_offset); 309,314c304,308 < if ( boundp ) < //right end of span with boundary < xr = (int)vcl_ceil( crossedges[curcrossedge+1].x - vgl_polygon_scan_iterator_offset); < else < // right end of span without boundary < xr = (int)vcl_floor( crossedges[curcrossedge+1].x - vgl_polygon_scan_iterator_offset); --- > if ( have_window && xl < irnd(win.min_x()) ) > { > fxl = win.min_x(); > xl = irnd(fxl); > } 316,319c310,329 < if ( have_window && xr >= irnd(win.max_x()) ) < { < fxr = win.max_x() -1; < xr = irnd(fxr); --- > if ( boundp ) > //right end of span with boundary > xr = (int)vcl_ceil( crossedges[curcrossedge+1].x - vgl_polygon_scan_iterator_offset); > else > // right end of span without boundary > xr = (int)vcl_floor( crossedges[curcrossedge+1].x - vgl_polygon_scan_iterator_offset); > > if ( have_window && xr >= irnd(win.max_x()) ) > { > fxr = win.max_x() -1; > xr = irnd(fxr); > } > > // adjust the x coord so that it is the intersection of > // the edge with the scan line one above current > crossedges[curcrossedge].x += crossedges[curcrossedge].dx; > crossedges[curcrossedge+1].x += crossedges[curcrossedge+1].dx; > curcrossedge+=2; > if ( xl <= xr ) > return true; 322,328c332,336 < // adjust the x coord so that it is the intersection of < // the edge with the scan line one above current < crossedges[curcrossedge].x += crossedges[curcrossedge].dx; < crossedges[curcrossedge+1].x += crossedges[curcrossedge+1].dx; < curcrossedge+=2; < if (! ( xl <= xr ) ) < return next(); --- > // All segments on current scan line have been exhausted. Start > // processing next scan line. > vertind curvert, prevvert, nextvert; > if ( y > y1 ) > return false; 330,331c338,340 < return true; < } --- > { > // Current scan line is not the last one. > bool not_last = true; 333,350c342,354 < // All segments on current scan line have been exhausted. Start < // processing next scan line. < vertind curvert, prevvert, nextvert; < if ( y <= y1 ) < { < // Current scan line is not the last one. < bool not_last = true; < < // If boundary included and processing first or last scan line < // floating point scan line must be taken as a min/max y coordinate < // of the polygon. Otherwise these scan lines are not included because < // of the earlier rounding (ceil/floor). < if ( boundp ) { < if ( y == y0 ) < fy = vcl_floor(get_y( yverts[ 0 ] )); < else if ( y == y1 ) { < fy = vcl_ceil(get_y( yverts[ numverts - 1 ] )); < not_last = false; --- > // If boundary included and processing first or last scan line > // floating point scan line must be taken as a min/max y coordinate > // of the polygon. Otherwise these scan lines are not included because > // of the earlier rounding (ceil/floor). > if ( boundp ) { > if ( y == y0 ) > fy = vcl_floor(get_y( yverts[ 0 ] )); > else if ( y == y1 ) { > fy = vcl_ceil(get_y( yverts[ numverts - 1 ] )); > not_last = false; > } > else > fy = T(y); 354,356d357 < } < else < fy = T(y); 358,377c359,378 < for (; k<numverts && get_y(yverts[k]) <= fy+vgl_polygon_scan_iterator_offset && not_last; k++) < { < curvert = yverts[ k ]; < < // insert or delete edges (curvert, nextvert) and (prevvert, curvert) < // from crossedges list if they cross scanline y < get_prev_vert( curvert, prevvert ); < < if ( get_y( prevvert ) <= fy-vgl_polygon_scan_iterator_offset) // old edge, remove from active list < delete_edge( prevvert ); < else if ( get_y( prevvert ) > fy+vgl_polygon_scan_iterator_offset) // new edge, add to active list < insert_edge( prevvert ); < < get_next_vert( curvert, nextvert ); < < if ( get_y( nextvert ) <= fy-vgl_polygon_scan_iterator_offset) // old edge, remove from active list < delete_edge( curvert ); < else if ( get_y( nextvert ) > fy+vgl_polygon_scan_iterator_offset) // new edge, add to active list < insert_edge( curvert ); < } --- > for (; k<numverts && get_y(yverts[k]) <= fy+vgl_polygon_scan_iterator_offset && not_last; k++) > { > curvert = yverts[ k ]; > > // insert or delete edges (curvert, nextvert) and (prevvert, curvert) > // from crossedges list if they cross scanline y > get_prev_vert( curvert, prevvert ); > > if ( get_y( prevvert ) <= fy-vgl_polygon_scan_iterator_offset) // old edge, remove from active list > delete_edge( prevvert ); > else if ( get_y( prevvert ) > fy+vgl_polygon_scan_iterator_offset) // new edge, add to active list > insert_edge( prevvert ); > > get_next_vert( curvert, nextvert ); > > if ( get_y( nextvert ) <= fy-vgl_polygon_scan_iterator_offset) // old edge, remove from active list > delete_edge( curvert ); > else if ( get_y( nextvert ) > fy+vgl_polygon_scan_iterator_offset) // new edge, add to active list > insert_edge( curvert ); > } 379,380c380,381 < // sort edges crossing scan line by their intersection with scan line < local_qsort(crossedges, numcrossedges); --- > // sort edges crossing scan line by their intersection with scan line > local_qsort(crossedges, numcrossedges); 382,387c383,386 < curcrossedge = 0; // Process the next set of horizontal spans < y++; < return next(); < } < else < return false; --- > curcrossedge = 0; // Process the next set of horizontal spans > y++; > } > } while ( true ); |
From: Amitha P. <ami...@us...> - 2007-12-24 15:21:27
|
(Both messages did get through, BTW.) My thoughts: I find unified diffs (diff -u) are far easier to read. On vil_jpeg_compressor, you set quality as a static int, implying then that all the writers will use the same quality. I think it's better for that variable not be static, and allow each writer to set its quality independently. The default can be set to 75 (as you have it) in the constructor. On j2k: I think the new signature is quite likely to conflict with existing uses, since the factors are likely to be integers in many applications, and folks will likely have written "2" instead of "2.0". Unfortunately, this would cause a "silent" error, and so I think we should avoid it. Maybe rename the new function to something else? I can't think of a good name. Maybe something along the lines of get_copy_view_decimated_explicit_size get_copy_view_decimated_by_size get_copy_view_explicit_size get_copy_view_decimated2 On vgl: good with me. Avoiding stack overflows seems to be a good idea. :-) (I assume the tests still pass.) Amitha. |
From: Brendan M. <bre...@gm...> - 2007-12-26 07:29:51
|
OK, I'll make those changes and then check in at some point ... On 25/12/2007, Amitha Perera <ami...@us...> wrote: > (Both messages did get through, BTW.) > > My thoughts: > > I find unified diffs (diff -u) are far easier to read. > > On vil_jpeg_compressor, you set quality as a static int, implying then > that all the writers will use the same quality. I think it's better for > that variable not be static, and allow each writer to set its quality > independently. The default can be set to 75 (as you have it) in the > constructor. > > On j2k: I think the new signature is quite likely to conflict with > existing uses, since the factors are likely to be integers in many > applications, and folks will likely have written "2" instead of "2.0". > Unfortunately, this would cause a "silent" error, and so I think we > should avoid it. Maybe rename the new function to something else? I > can't think of a good name. Maybe something along the lines of > get_copy_view_decimated_explicit_size > get_copy_view_decimated_by_size > get_copy_view_explicit_size > get_copy_view_decimated2 > > On vgl: good with me. Avoiding stack overflows seems to be a good idea. > :-) (I assume the tests still pass.) > > > Amitha. > > -- Cheers, Brendan |