Fix animation errors #2898

bytbox · 2014-03-14T09:15:59Z

Three changes: use 'y#' in python 3, ignore errors from AsFileDescriptor (to allow use of BytesIO and similarly brain-dead objects as the target), and don't require setting of the position of the file handle to succeed (so that we can use streams).

Fixes #1891

mdboom · 2014-03-14T12:49:30Z

src/_backend_agg.cpp

        PyObject_CallFunction(write_method, (char *)"s#", pixBuffer, NUMBYTES);
+        #endif


Good catch.

tacaswell · 2014-03-14T12:53:58Z

Before this gets merged some of the changes in #2838 should be reverted so that the smoke tests run again.

mdboom · 2014-03-14T12:56:28Z

src/file_compat.h

@@ -70,6 +70,7 @@
    Py_DECREF(ret);
    fd = PyObject_AsFileDescriptor(file);
    if (fd == -1) {
+        PyErr_Clear();


I'm not sure this is the right solution. I think there are cases where you might want to return this error to the Python side. In cases where one doesn't, we are already calling PyErr_Clear in _png.cpp after calling mpl_PyFile_Dup. I think _backend_agg.cpp should also do that -- but I don't think it makes sense to do that here. The idea of this function is that a Python error should always be set when in returns NULL (like most Python/C API functions).

mdboom · 2014-03-14T12:58:35Z

Thank for this!

…lotlib#1891.

bytbox · 2014-03-14T16:00:25Z

Fixes made. #2838 is not in v1.3.x, so I'm not reverting those changes here. Should I do a separate PR on master for that?

tacaswell · 2014-03-14T16:04:23Z

Sorry, didn't notice where this was going. On 1.3.x the smoke tests are never run so it's not a 'problem'.

1.3.x will get merged into master so defiantly don't repeat the full PR. If you want to do a PR for reverting the KnownFail changes please do, if not leaving a note for who ever merges this to clean it up should be enough.

bytbox · 2014-03-14T16:19:19Z

Ok, #2900 takes care of the KnownFail.

tacaswell · 2014-03-18T01:17:16Z

@mdboom This outside of my comfort zone to approve, if you are happy can you merge this?

tacaswell · 2014-03-18T01:22:22Z

on further digging it looks like there are really two animation bugs with, the smoketest failures are related to #2568, not #1891 .

bytbox · 2014-04-15T00:26:12Z

Ping? (Don't want to break with etiquette too badly, but I'd like to see this merged in :)

tacaswell · 2014-04-15T12:07:34Z

@bytbox Don't worry about pestering. This is marked to be dealt with before 1.4 is cut.

@mdboom Can you take another look at this?

efiring · 2014-05-11T20:13:39Z

My impression is that this PR has been vetted by @mdboom, and solves a problem up through Python 3.3, so maybe it is worth merging it now, into 1.3.x and from there into 1.4.x. If it is confirmed that there is a problem with 3.4, that can be treated separately.

efiring · 2014-05-11T23:24:17Z

As noted in my comment on #1891, this PR needs one more change: move the Py_DECREF(ret) on line 160 of file_compat.h up one level, so we have

        /* Seek Python-side handle to the FILE* handle position */
        ret = PyObject_CallMethod(file, "seek", MPL_OFF_T_PYFMT "i", position, 0);
        if (ret == NULL) {
            return -1;
        }
        Py_DECREF(ret);
    }
    return 0;

That will ensure Py_DECREF is called if and only if ret is not NULL.

bytbox · 2014-05-13T02:19:53Z

Fixed.

efiring · 2014-05-13T03:41:25Z

@bytbox I don't know why Travis failed on Python 2, but it seems unrelated. (It looks familiar, and I think an explanation has been given, but I don't remember what it is.)

Would you fix the indentation of the Py_DECREF line, please? It should be 8 spaces (no tabs) so that it lines up with the } above.
Thank you.

bytbox · 2014-05-13T12:59:36Z

Oops - fixed the indentation.

On Mon, 12 May 2014, Eric Firing wrote:

@bytbox I don't know why Travis failed on Python 2, but it seems unrelated. (It looks familiar, and I think an explanation has been given, but I don't remember what it is.)

Would you fix the indentation of the Py_DECREF line, please? It should be 8 spaces (no tabs) so that it lines up with the } above.
Thank you.

Reply to this email directly or view it on GitHub:
#2898 (comment)

Scott Lawrence

Fix animation errors

efiring · 2014-05-13T18:22:42Z

@tacaswell, will you take care of merging this into 1.4.x? And, if we are not going to release another 1.3.x, let's stop merging anything into it.

tacaswell · 2014-05-13T23:55:03Z

I think we have one or two other PRs that are against 1.3.x, it is probably not worth rebasing them.

I will merge this in tonight.

tacaswell · 2014-05-19T03:03:28Z

I merged 1.3.x today so this should be taken care of.

mangecoeur · 2014-07-25T11:06:17Z

Just run into this bug - is this in 1.3 series or not in the end, seems the issue has not been closed...

tacaswell · 2014-07-25T13:01:18Z

@mangecoeur Can you check using current master or the 1.4.0rc1 code? The fix for this was done after 1.3.1 was cut.

mangecoeur · 2014-07-25T15:24:05Z

@tacaswell can't change versions right now for various reasons, but I'm on 1.3.1 so that explains it... will wait for 1.4 release...

mdboom reviewed Mar 14, 2014
View reviewed changes

src/_backend_agg.cpp

PyObject_CallFunction(write_method, (char *)"s#", pixBuffer, NUMBYTES);

#endif

Copy link

Member

mdboom Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

mdboom reviewed Mar 14, 2014
View reviewed changes

Use 'y#' instead of 's#' in write_rgba of the agg backend. Fixes matp…

7dc06fb

…lotlib#1891.

bytbox mentioned this pull request Mar 14, 2014

Remove no-longer-necessary KnownFail for python 3.2. #2900

Merged

tacaswell modified the milestones: v1.4.0, v1.3.x Mar 18, 2014

palmieri mentioned this pull request Apr 16, 2014

Animation module errors out when using Python3 #1891

Closed

tacaswell modified the milestones: v1.4.0, v1.3.x Apr 17, 2014

tacaswell added the needs_review label Apr 17, 2014

Prevent mpl_PyFile_Dup from failing when given BytesIO or similar

b79541d

efiring added a commit that referenced this pull request May 13, 2014

Merge pull request #2898 from bytbox/v1.3.x

2b74cb5

Fix animation errors

efiring merged commit 2b74cb5 into matplotlib:v1.3.x May 13, 2014

QuLogic removed the status: needs review label Nov 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix animation errors #2898

Fix animation errors #2898

bytbox commented Mar 14, 2014

mdboom Mar 14, 2014

tacaswell commented Mar 14, 2014

mdboom Mar 14, 2014

mdboom commented Mar 14, 2014

bytbox commented Mar 14, 2014

tacaswell commented Mar 14, 2014

bytbox commented Mar 14, 2014

tacaswell commented Mar 18, 2014

tacaswell commented Mar 18, 2014

bytbox commented Apr 15, 2014

tacaswell commented Apr 15, 2014

efiring commented May 11, 2014

efiring commented May 11, 2014

bytbox commented May 13, 2014

efiring commented May 13, 2014

bytbox commented May 13, 2014

efiring commented May 13, 2014

tacaswell commented May 13, 2014

tacaswell commented May 19, 2014

mangecoeur commented Jul 25, 2014

tacaswell commented Jul 25, 2014

mangecoeur commented Jul 25, 2014

		PyObject_CallFunction(write_method, (char *)"s#", pixBuffer, NUMBYTES);
		#endif

Fix animation errors #2898

Fix animation errors #2898

Conversation

bytbox commented Mar 14, 2014

mdboom Mar 14, 2014

Choose a reason for hiding this comment

tacaswell commented Mar 14, 2014

mdboom Mar 14, 2014

Choose a reason for hiding this comment

mdboom commented Mar 14, 2014

bytbox commented Mar 14, 2014

tacaswell commented Mar 14, 2014

bytbox commented Mar 14, 2014

tacaswell commented Mar 18, 2014

tacaswell commented Mar 18, 2014

bytbox commented Apr 15, 2014

tacaswell commented Apr 15, 2014

efiring commented May 11, 2014

efiring commented May 11, 2014

bytbox commented May 13, 2014

efiring commented May 13, 2014

bytbox commented May 13, 2014

efiring commented May 13, 2014

tacaswell commented May 13, 2014

tacaswell commented May 19, 2014

mangecoeur commented Jul 25, 2014

tacaswell commented Jul 25, 2014

mangecoeur commented Jul 25, 2014