bugGNU Octave - Bugs: bug #44406, glps_renderer::draw uses static...

 
 

bug #44406: glps_renderer::draw uses static state variable, potential bug?

Submitter:  Dan Sebald <sebald>
Submitted:  Mon 02 Mar 2015 07:07:45 AM UTC
   
 
Category:  Plotting with OpenGL Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Other
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 03 Mar 2015 09:07:16 PM UTC, comment #7: 

Follow up on some of the issues in https://savannah.gnu.org/bugs/?44406#comment6.

I've made a reference to this "Control-C while printing" behavior for Qt in this bug report:

https://savannah.gnu.org/bugs/?44387

That bug report isn't the exact same issue, but it is closely related and it might make sense to address both at the same time.

I made a separate bug report for issue #1, https://savannah.gnu.org/bugs/?44415, initially simply to determine if the vertical lines is an OpenGL bug.

I've also made a separate bug report for issue #2, https://savannah.gnu.org/bugs/?44416, as supposedly Qt 4.8 will allow QPainter in a separate thread to make OpenGL perform better.

Dan Sebald <sebald>
Tue 03 Mar 2015 08:08:54 AM UTC, comment #6: 

I'm not sure either.  I've tried to investigate a before-and-after for changeset 19889, but it seems like the same behavior in both cases.

I create a big image with


image(32*rand(5000))


and type Cntrl-C once or twice while OpenGL is working.  I couldn't cause anything real strange with repeated tries.

On the other hand, following up with this command


print -deps test.eps


is a different story.  Pressing Cntrl-C when the printing process is taking place can easily put Octave into a bad state.  The plot can be shredded.  The command line can be unresponsive.  The whole of the program can be in a sleep state.

Couple other things

1) With that test image displayed, I'm seeing a vertical grey line at about x=4100.  I'm not sure how that can be an Octave problem.  More likely a bug in OpenGL implementation.  I assume your versions of OpenGL are different from mine, so If you can see a similar line, then maybe I'll create a bug report.

2) I see that the rendering for the plot image is handled inside the main process.  Moving the main window around will cause the slow 5000x5000 image redraw, but that also makes Octave GUI very unresponsive.  It would be nice if the portion of OpenGL that does the video graphics memory update could be in a separate process.  Too much to ask of OpenGL, I suppose.

Dan Sebald <sebald>
Tue 03 Mar 2015 02:35:45 AM UTC, comment #5: 

It's not clear to me whether an interrupt could actually cause trouble in the glps_renderer::draw function, but I went ahead and checked in this changetset anyway:

http://hg.savannah.gnu.org/hgweb/octave/rev/00c3f2021cad

John W. Eaton <jwe>
Group administrator
Mon 02 Mar 2015 06:36:47 PM UTC, comment #4: 

Also...

In Octave code, interrupts only set a global variable which is checked periodically.  It's only at the point of checking the global interrupt state variable that the interrupt actually happens.  So unless the code that is protected with the static variable calls something that checks the global variable and throws an interrupt exception (usually with the OCTAVE_QUIT macro) then there is no need to use unwind_protect.  But you should only skip the unwind_protect if you are certain that there can be no calls to OCTAVE_QUIT where the value of the static (or global) varaiable is set/reset.

John W. Eaton <jwe>
Group administrator
Mon 02 Mar 2015 06:25:26 PM UTC, comment #3: 

"so unwind_protect destructor would be called in that case too"

OK, got it, thanks.

Dan Sebald <sebald>
Mon 02 Mar 2015 02:33:47 PM UTC, comment #2: 

An unwind_protect object is just a C++ object that stores data and associated actions.  It doesn't suspend signal delivery.

The unwind_protect destructor will execute at the end of the enclosing scope, then whatever actions have been attached to it will be performed at that time.  Interrupts ultimately cause a C++ exception to be thrown, so unwind_protect destructor would be called in that case too.

The unwind_protect object in the scope that calls glps_renderer::draw doesn't do anything about restoring the value of the static variable in_draw.  It looks to me like an unwind_protect object is needed there too.

John W. Eaton <jwe>
Group administrator
Mon 02 Mar 2015 09:18:30 AM UTC, comment #1: 

Maybe I'm missing something.  Is this unwind_protect:


              if (filep)
                {
                  unwind_protect frame;

                  frame.add_fcn (close_fcn, filep);

                  glps_renderer rend (filep, term);
                  rend.draw (fobj, "");

                  // Make sure buffered commands are finished!!!
                  glFinish ();
                }


meant to protect the hunk of code after it and until it goes out of scope restricted from interruption by Cntrl-C?

Dan Sebald <sebald>
Mon 02 Mar 2015 07:07:45 AM UTC, original submission:  

In searching for a bug, I've looked through gl2ps-renderer.cc.  I notice something here about the routine glps_renderer().  I'm going to raise it because if this code is interruptable, there is potential for a rare obscure bug that would be very hard to duplicate and find if some user were to report it.

There is this construct:


glps_renderer::draw (const graphics_object& go, const std::string print_cmd)
{
  static bool in_draw = false;

  if (!in_draw)
    {
      in_draw = true;
...

  in_draw = 0;
  ret
}


If this code can be interrupted, there is the rare event possibility that the user hits Cntrl-C at a time that corresponds to the in_draw variable value being 1.  If so, from that point forward this draw routine will not behave correctly if the user follows up with some related command.  Very rare, I know, but rare bugs are the worst kind.

So, if that is an issue, how to fix?  Well, there should be some way of reseting that static variable, or more likely make it a private class variable and reset it to zero every time it is first used.  Keep the recursive portion separate from the.


public
  draw();
private
  draw_recurse();
  int in_draw_rec;



  ::draw() {
    in_draw_rec = 0;
    draw_recurse();
  }

  ::draw_recurse() {
    in_draw_rec = 1;
    draw_recurse(go);
  }


one could do the same sort of thing with an input value:


  ::draw() {
    draw_recurse(go, 0);
  }

  ::draw_recurse(xyz, state) {
    draw_recurse(go, state+1);
  }


I may not have that right, but I'm not quite sure how it is intended to work because of the virtual functions and all.  There might be a better way of doing this as well.  I haven't thought too much about it.

Dan Sebald <sebald>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by sebald (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-03-03 jwe StatusNone Fixed
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-bb6a.
    Corresponding source code