<div class="gmail_quote">On Tue, Feb 19, 2008 at 3:32 AM, Behdad Esfahbod <<a href="mailto:behdad@behdad.org">behdad@behdad.org</a>> wrote: <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> On Sat, 2008-02-16 at 00:50 +0200, Iosif Haidu wrote: >> Hi, <div class="Ih2E3d"> >Hi, >> I have recently experienced an interesting behavior of the >> 'cairo_restore()' function. It doesn't restore the cairo context saved >> by 'cairo_save()' if an error occurs when executing windows gdi >> functions. >> >> When I used these two functions I assumed that they must provide a >> transaction like functionality: </div>> Wrong assumption.  And of course wrong documentation on our side.  What > cairo_save/restore do is save/restore all the settings/parameters of the > context except for groups, current path, and of course status.  In > postscript speak, it saves/restores the graphics state (sans the path). <div class="Ih2E3d"></div></blockquote><div> I agree with you that my assumption might be wrong, but that's what happens when function names are not chose wisely (according with their implementation). If the 'cairo_restore()' function would have been named 'cairo_partial_restore()' instead, then I would have know that probably only some of the fields of 'cairo_t' structure will be restored and not all of them. Do you consider that windows device context handle is not worth to be restored ? ('dc' member of 'cairo_win32_surface_t') </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> <div> >>  'cairo_save()' saves the cairo context while 'cairo_restore()' would >> restore it even if an error would occur by executing cairo functions >> between. But this is not the case. If a cairo function fail to execute >> between,  by failing one of the windows gdi function, then >> 'cairo_restore()' will not restore the previously saved context. >> I have experienced such situation for the application I'm working on >> it. For an unknown reason sometime some windows gdi functions >> (CreateCompatibleDC(), BitBlt() and other) fail to execute with error >> code 6 and 8. In this situation the cairo function responsible with >> that windows gdi function call will set the error status to >> CAIRO_STATUS_NO_MEMORY and the 'cairo_restore()' will not restore the >> cairo context. I have noticed that cairo context actually has not been >> damaged by the failure of the windows gdi function, yet the context is >> not restored. </div></blockquote><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="Ih2E3d"> </div>> As I said on the bug, the this is a bug that should be fixed.  If the > failing gdi functions are not fatal, this should be handled in cairo. > If the errors are fatal errors (say, real out-of-memory), cairo fails to > produce the exact output that you asked it to, and that's an error.  And > cairo's error handling model is to retain errors.  It's really that > simple.  <div class="Ih2E3d"></div></blockquote><div> Well, the output of the error is that my application main window freezes and nothing is drawn anymore (I must close the application and start again). I would say that this error it's a critical one. The problem why the main screen freezes is because the windows device context handle is not restored and somehow inside the library this is damaged. For an application that draws real time waveforms it would be acceptable to lose some drawings but not to freeze. Probably you didn't try to check the cairo library functions and try some scenario of 'what can happen if xxx windows gdi function fails for some unknown reason'. But I can give you some examples: a) File: cairo-win32-surface.c Function: static cairo_status_t _create_dc_and_bitmap(...) ... surface->dc = CreateCompatibleDC(original_dc); if (!surface->dc) goto FAIL; surface->bitmap = CreateDIBSection (surface->dc,                             bitmap_info,                             DIB_RGB_COLORS,                             &bits,                             NULL, 0); if (!surface->bitmap) goto FAIL; ... surface->saved_dc_bitmap = SelectObject (surface->dc,                          surface->bitmap); if (!surface->saved_dc_bitmap) goto FAIL; ... FAIL:     status = _cairo_win32_print_gdi_error ("_create_dc_and_bitmap");     ...     if (surface->saved_dc_bitmap) {     SelectObject (surface->dc, surface->saved_dc_bitmap);     surface->saved_dc_bitmap = NULL;     }     In my situation, for unknown reason, the execution of 'CreateCompatibleDC()' fails. I'll let you to find out what happens in the FAIL section if 'CreateCompatibleDC()' or 'CreateDIBSection()' fails. b) File: cairo-win32-surface.c Function: static cairo_status_t _cairo_win32_surface_acquire_dest_image() ... if (GetClipBox (surface->dc, &clip_box) == ERROR) return _cairo_win32_print_gdi_error ("_cairo_win3_surface_acquire_dest_image"); ... The execution of 'GetClipBox()' sometimes fails. c) File: cairo-win32-surface.c Function: static cairo_int_status_t _cairo_win32_surface_set_clip_region() ... if (ExtSelectClipRgn (surface->dc, gdi_region, RGN_AND) == ERROR) status = _cairo_win32_print_gdi_error ("_cairo_win32_surface_set_clip_region"); .. The execution of 'ExtSelectClipRgn()' sometimes fails. And here are the cairo api leading to this situation:     cairo_save(pDcM);     cairo_move_to(pDcM, rBeginP.getX(), rBeginP.getY());     cairo_line_to(pDcM, rEndP.getX(), rEndP.getY());     cairo_stroke(pDcM);     cairo_restore(pDcM); Now, if in the first case it's clearly a bug to not check in the FAIL section if 'surface->saved_dc_bitmap' has a valid value (it has a garbage value != 0 and it goes inside the 'if' and that garbage value is selected in the surface device context) for the other two cases I'll let you to find out the real reason. You could say that in the first case the bug can be fixed, so probably the problem will be also fixed. But fixing the 'cairo_restore()' to really restore what have been saved has also other benefits. For example such bugs will not affect anymore the main application (there are other mechanisms to catch such bugs if this is the intention of retaining the error code), the execution of a cairo_save/cairo_restore block will act like a transaction (transactions are a good mechanism to build a stable systems) > What is it good for if cairo tells you that everything is ok > but in reality it failed to draw some of the stuff you asked it to? I never said that. I think you misunderstood my explanation or didn't pay attention enough on my previous e-mail. The developer is able to check every moment, inside of a 'cairo_save()/cairo_restore()' block if an error has occurred by calling 'cairo_status()' api function and act correspondingly.  What I have said was that is not necessary to keep the error but rather clear it after 'cairo_restore()' will finish it's execution. The only drawback of this fix is compatibility with previous versions. And yes, there might be applications that would rather like to miss some drawings but instead restore the right device context handle. </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="Ih2E3d"> >> Also the cairo documentation doesn't mention that 'cairo_restore()' >> will not restore the context if an error occurs in a cairo function >> between 'cairo_save()' and 'cairo_restore()'. </div>> When a cairo object is in error, it shuts down.  No further operation on > it works normally.  This is to ensure that no malicious behavior is > caused by an erroneous object.</blockquote><div> If you will continue to follow the code execution of those examples I gave, you'll see that actually the fields of cairo context are not damaged or set to an inconsistent values. Just follow the execution path. </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> <div class="Ih2E3d"> >> I would like to propose a change in the 'cairo_restore()' >> implementation, such that: >>   a) will restore the previously saved context by 'cairo_save()' even >> if in an error occurs >>   b) will return as result the error code generated by any cairo >> function executed between 'cairo_save()' and 'cairo_restore()' >>   c) will reset the error status to CAIRO_STATUS_SUCCESS so that any >> next execution of 'cairo_save()' and 'cairo_restore()' will not exit >> because of a previous status set to an error code. </div>> So, no.  This doesn't make any sense to me.  What does make some sense > is to add some new api, for example cairo_clear_status().  But > implementing that kind of API is almost impossible without really > drastic changes to cairo internals. > I do see where you are coming from though.  I was myself very surprised > and puzzled by the fact that this can put the cairo_t in a > non-recoverable error state: >  cairo_save (cr); >  cairo_scale (cr, 0, 0); >  cairo_restore (cr); > But my solution to it is not what suggest.  My proposed solution is that > errors should not be generated until there's really no other choice.  In > the above case, there's nothing wrong with the code semantically.  For > one reason, it's not drawing anything with the degenerate matrix, so it > can't be an error.  Moreover, drawing with a degenerate matrix is quite > well-defined and useful too.  So, these cases are false positives in > current cairo implementation that should be fixed, instead of hacked > around.  From what you described, your case is similar IMO.</blockquote><div> Well I disagree with your solution  for the following reason:   - it's not easy to implement it   - it's not wise to let the developer to worry when it needs to reset the error code or not. Another thing to worry about. </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> <div class="Ih2E3d"> >> The function would be something like: >> >> int >> cairo_restore (cairo_t *cr) >> { >> ... >> int result = CAIRO_STATUS_SUCESS; >> if (cr->status != CAIRO_STATUS_SUCCESS) >> { >>     result = cr->status; >>     cr->status = CAIRO_STATUS_SUCCESS; >>     if ((cr->gstate) && (cr->gstate->target)) >>     { >>         cr->gstate->target->status = CAIRO_STATUS_SUCCESS; </div>> Any and all of cr, cr->gstate, and cr->gstate->target may be in an > inconsistent state at this point.  Reseting their status to SUCCESS > doesn't magically make them continue working. <div><div></div><div class="Wj3C7c"></div></div></blockquote><div> Actually, yes. Resetting their status to SUCCESS will magically make them continue working, because nothing is damaged in the cairo context structure if a windows gdi function fails. </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div class="Wj3C7c"> >     } > } > // if (cr->status) > // return; > ... > return result; > } > > > Best regards, > Iosif Haidu </div></div>-- behdad <a href="http://behdad.org/" target="_blank">http://behdad.org/</a> "Those who would give up Essential Liberty to purchase a little  Temporary Safety, deserve neither Liberty nor Safety."        -- Benjamin Franklin, 1759 </blockquote></div> Best regards, Iosif Haidu