Re: Glass Robot and getSCreenCapture
It should be throwing an exception or returning null, returning a real value is a code smell. Sent from my iPhone On 23 Mar 2014, at 07:55, Daniel Blaukopf daniel.blauk...@oracle.com wrote: We should be consistent about what we return, although I agree that that the actual value doesn’t seem to matter. 0 for imaginary pixels seems reasonable. On Mar 21, 2014, at 7:05 PM, Anthony Petrov anthony.pet...@oracle.com wrote: Hi David, I don't think we're making any assumptions. We feed the coordinates to a native API and rely on the OS to do the right thing. In other words, our assumption is that if the box lays (partially or fully) outside of the screen area, then the behavior is undefined. Note that the Screen API in Glass allows its clients to check what coordinates are valid (i.e. belong to a real screen). So whatever your return for pixels outside of screen bounds should be fine. 0x0 or 0xff00 - both look good. However, I agree that a stricter specification and a check might be the best solution. -- best regards, Anthony On 3/21/2014 8:53 PM, David Hill wrote: I have been working on a problem with Robot.getScreenCapture() on a 565 ARM device, and while doing so, encountered a couple of questions which I will bring up: Pixxls getScreenCapture(int x, int y, int width, int height, boolean isHiDPI) I don't seem any real documentation that says how x,y + width,height should be treated when compared to the reality of the Screen. What values of x,y + width,height are reasonable ? I can picture any number of scenarios with them that would result in a box that does not fit within the Screen dimensions. The only implementing code I have seen checks to that the width height are = 1. Can I/Should I handle -x values ? What if the requested bounds exceed the screen ? If we are making assumptions that the requested box is inside the screen then why don't we document that fact and add a check in the Robot class (instead of relying on the native impls). If we are assuming the requested box does not have to lie within the screen bounds - what should the returned values be for the pixels outside the screen ? Pixel Black ? (Currently I think Lens would return 0x instead of 0xff00) My recommendation would be modify the JavaDoc contract to specify that the x,y and x+width, y+height must be within the screen bounds, with an IllegalArgumentException if out of bounds. This would be checked in Robot, prior to calling the native impls. This code is internal API, so I expect the real impact would be on SQE.
hg: openjfx/8u-dev/rt: RT-36371 GridPane ColumnConstraints Java 7 and 8 broken compatibility
Changeset: 14580ce9a75e Author:Martin Sladecek martin.slade...@oracle.com Date: 2014-03-26 09:39 +0100 URL: http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/14580ce9a75e RT-36371 GridPane ColumnConstraints Java 7 and 8 broken compatibility ! modules/graphics/src/main/java/javafx/scene/layout/GridPane.java ! modules/graphics/src/test/java/javafx/scene/layout/GridPaneTest.java
Re: Adding GStreamer plugins
Hi David, Sure - I'm aware there's legal issues surrounding many of the formats, though one of the reasons I picked MKV to start with was because it's an open container format. I'm certainly not aware of any restrictions surrounding it (though please correct me if I'm wrong on that front!) A switch certainly sounds like a good idea though, especially for formats with more restrictions surrounding their use; I'll bear that in mind if I add any additional plugins. Thanks, Michael On 26 March 2014 02:03, David DeHaven david.deha...@oracle.com wrote: I mainly wanted to do this as a personal exercise, though I'd imagine this is a useful piece of functionality for many others also - so should I submit a patch of the changes, or is this unlikely to be accepted? (Again, sorry for the perhaps obvious question, I'm rather new to this.) I've had a look at the code review process and it seems to suggest adding a patch to the relevant JIRA issue for those who don't have commit access (in this case 18009), but I don't seem to have permission to do that either? It sounds like there are perhaps two different ways to move forward here, the first is to take a submission in the form of WIKI writeup that we can post so that anybody else who wants to try extending the media files can follow the path you took. The other is to take a patch for the given JIRA issue. Sadly, the JIRA server doesn't allow just anybody to supply patches, so you will have to email to Steve or Kevin and they can attach it to the issue for you. Be careful how these are implemented. We cannot just enable formats in GStreamer, there are licensing and legal issues involved. I think the Matroska licenses are fairly benign, but it still requires some amount of process. What I would recommend is adding a switch to optionally enable additional formats, so those building GStreamer or OpenJFX themselves can turn whatever they want on or off, and they are on their own for dealing with legal issues. That being said, we actually tested with MKV during development and it was pretty solid :) -DrD-
hg: openjfx/8u-dev/rt: RT-35263: Win: Crashing VM in a JavaFX 3D app reading and writing STL files
Changeset: 89ca2db487e3 Author:Anthony Petrov anthony.pet...@oracle.com Date: 2014-03-26 15:13 +0400 URL: http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/89ca2db487e3 RT-35263: Win: Crashing VM in a JavaFX 3D app reading and writing STL files Summary: Check if the window is still alive after handling a synthetic mouse ENTER event Reviewed-by: snorthov, fheidric ! modules/graphics/src/main/native-glass/win/ViewContainer.cpp
Re: Adding GStreamer plugins
There is another issue with new plugins. We have thoroughly tested with a closed source internal test suite on all platforms every plugin that we already have. If we add something new this may open a way for crashes, security breaches. So if you add something new and want this to be included in Oracle JavaFX (JDK) this needs to be tested well. As for OpenJFX - it's a good starting point. On 26.03.2014 12:58, Michael Berry wrote: Hi David, Sure - I'm aware there's legal issues surrounding many of the formats, though one of the reasons I picked MKV to start with was because it's an open container format. I'm certainly not aware of any restrictions surrounding it (though please correct me if I'm wrong on that front!) A switch certainly sounds like a good idea though, especially for formats with more restrictions surrounding their use; I'll bear that in mind if I add any additional plugins. Thanks, Michael On 26 March 2014 02:03, David DeHaven david.deha...@oracle.com wrote: I mainly wanted to do this as a personal exercise, though I'd imagine this is a useful piece of functionality for many others also - so should I submit a patch of the changes, or is this unlikely to be accepted? (Again, sorry for the perhaps obvious question, I'm rather new to this.) I've had a look at the code review process and it seems to suggest adding a patch to the relevant JIRA issue for those who don't have commit access (in this case 18009), but I don't seem to have permission to do that either? It sounds like there are perhaps two different ways to move forward here, the first is to take a submission in the form of WIKI writeup that we can post so that anybody else who wants to try extending the media files can follow the path you took. The other is to take a patch for the given JIRA issue. Sadly, the JIRA server doesn't allow just anybody to supply patches, so you will have to email to Steve or Kevin and they can attach it to the issue for you. Be careful how these are implemented. We cannot just enable formats in GStreamer, there are licensing and legal issues involved. I think the Matroska licenses are fairly benign, but it still requires some amount of process. What I would recommend is adding a switch to optionally enable additional formats, so those building GStreamer or OpenJFX themselves can turn whatever they want on or off, and they are on their own for dealing with legal issues. That being said, we actually tested with MKV during development and it was pretty solid :) -DrD-
Re: Adding GStreamer plugins
Michael, On 26.03.2014 04:11, Michael Berry wrote: Kirill - I think I'll take your suggestion next and start looking at upgrading the existing native components to the latest version of GStreamer before I look at adding any more plugins, that would seem to make sense. Have you any pointers in how best to approach this? No pointer at all. I have it my my head. And it's about time to pass this experience before I forget it =) The thing is I did it once and it took me ~ 2 months. Here is the brief plan that you need to keep: [1] Start with the lower lever. It's glib. Linux doesn't need any glib update - we use the system glib. So glib update is necessary for Win/Mac. [1.1] Latest glib has plenty of dependencies on other 3-rd party libraries. But there is one that's mandatory - libFFI. Fortunately Oracle has approval to use libFFI in it's products. But this probably isn't necessary for OpenJFX. So at you first step you need to bring libFFI sources, place them in rt/modules/media/src/main/native/gstreamer/3rd_party/libffi and make sure it builds on Windows and Mac producing lib/a for static linking. You can probably make dll/dylib instead but I don't think it's necessary. [1.2] Take the latest glib 2.38 or even 2.40. Place them in rt/modules/media/src/main/native/gstreamer/3rd_party/glib Note that you don't need all sources/headers. But to remove precisely what's redundant you should first compile gstreamer/plugins. Here you make sure you can compile and build glib-lite.dll/libglib-lite.dylib Having done 1.2 you should be able to run media component with the new glib-lite.dll. If it runs then you're done with glib upgrade. It's important to apply fixes that we made in glib to the newer glib library. You can find them by grepping for GSTREAMER_LITE in sources/headers. [2] GStreamer update. [3] Oracle plugins compilation/update. This step will also be necessary because 0.10.35 API is different from 1.0. For Example GstBuffer that we extensively use has incompatible APIs. I won't get deeper into details for [2] and [3] now. Let's just handle [1] and then continue.
Re: Adding GStreamer plugins
https://javafx-jira.kenai.com/browse/RT-18009 This JIRA is covering adding support for more media formats. Since we are just talking about MKV, please open a separate JIRA to cover this work and attach the patch there. We can link to the new JIRA from RT-18009. Steve On 2014-03-25 7:47 PM, Jonathan Giles wrote: Typically in this case you would email the patch to the assigned developer, but it appears RT-18009 is unassigned at present. I think that is the first hurdle that needs to be resolved, but if you email me your patch I will attach it to the jira issue so that it is at least available for others. Thanks! -- Jonathan On 26/03/2014 12:39 p.m., Michael Berry wrote: Hi all, Turns out it was a stupid mistake on my part causing the last few linking errors, I hadn't added one of the C files I needed to the makefile (well I had, but I'd then reverted it again without realising!) Thanks to all for the prods and advice. Once I'd done that, the build went through without a hitch - and then it was just a case of making the relevant additions to the native code to register the MKV type and create a pipeline from it using the plugin. This wasn't hard to work out at all; I've since tried several test files in MKV format (with AAC audio) all of which play in MediaPlayer without a hitch! I mainly wanted to do this as a personal exercise, though I'd imagine this is a useful piece of functionality for many others also - so should I submit a patch of the changes, or is this unlikely to be accepted? (Again, sorry for the perhaps obvious question, I'm rather new to this.) I've had a look at the code review process and it seems to suggest adding a patch to the relevant JIRA issue for those who don't have commit access (in this case 18009), but I don't seem to have permission to do that either? Thanks, Michael On 25 March 2014 17:01, Stephen F Northover steve.x.northo...@oracle.comwrote: On 2014-03-25 7:00 AM, Kirill Kirichenko wrote: Hi Michael. See my comments inline. On 24.03.2014 04:31, Michael Berry wrote: I'm now a bit further along with this, though struggling to get the matroska plugin to compile (getting a bunch of unresolved external symbol errors for functions it uses in glib - not entirely sure why at the moment, as I said C is not my strong point.) I've also noticed the plugins currently bundled have quite a few changes to the gstreamer version, and I can't quite work out the logic behind why things have been changed the way they have - so even after the compilation issue is resolved I'm now less confident that it will just drop in and work! Again, if someone knowledgeable in this area that's lurking in the shadows could shed any light on any of this, it would be hugely appreciated. We did some changes in existing GStreamer code because it had errors and because we needed to expand some functionality. The changes are not very extensive. However, putting the current problems aside for a bit, the snags I hit up until this point could I think be relatively easily addressed in the documentation. With that in mind could I suggest a few additional points for the wiki? These may be obvious to the majority reading here, but as someone completely new to building JFX they had me stumped for a while! I can give you some directions. There is no wiki. I'd appreciate if you created one. - It turns out that the Gstreamer stuff doesn't compile at all by default, which is why I wasn't seeing any changes on the native level. To ensure GStreamer is actually compiled, you need to copy the gradle.properties.template file to gradle.properties, and uncomment the #COMPILE_MEDIA = true line. (A similar scenario would appear to exist for any webkit alterations as per the line above.) You don't need to comment/uncomment anything. Just add -PCOMPILE_MEDIA=true to the gradle command line. You can however change the properties file too. - As well as the requirements listed, I needed the Windows SDK ( http://www.microsoft.com/en-gb/download/details.aspx?id=8279) installed for GStreamer to compile successfully under Windows (7) - without it cygpath just threw a rather confusing error. You need windows 7.1a SDK and speaking precisely you need only samples from it because samples have BaseClasses at Samples/multimedia/directshow/baseclasses. BaseClasses are used for Oracle direchshowwrapper plugin. - The developer workflow page ( https://wiki.openjdk.java.net/display/OpenJFX/Developer+Work+Flow) refers to -Djavafx.ext.dirs - I think this should be -Djava.ext.dirs instead? What are you gonna use it for ? I updated the page and got rid of the BINARY_STUB reference that is no longer needed. I'm happy to make the above changes myself but unsure of if / where you can sign up for an account, so I'm just throwing them here for now - if anyone could point me to the right place then that'd be great! Steve. Can you give an advice ?
file attachment in Jira
Hi, I know this has been discussed before, in one instance it was said file attachment in jira would not be possible due to internal possibles, later it was said this should be possible and would be dealt with. I think it would be important for this to be possible given the highly collaborative nature of the javafx project. This certainly is a hurdle to people who want to collaborate. IMHO attaching text/image files doesn't involve security risks. Thanks, best regards, -- Pedro Duque Vieira
Re: Adding GStreamer plugins
I added a wiki link: https://wiki.openjdk.java.net/pages/viewpage.action?pageId=18808963 I welcome free will participants to go over the steps, upgrade GStreamer and eventually create a precise version of the guide. I will provide as much help as I can. K On 26.03.2014 16:43, Kirill Kirichenko wrote: Michael, On 26.03.2014 04:11, Michael Berry wrote: Kirill - I think I'll take your suggestion next and start looking at upgrading the existing native components to the latest version of GStreamer before I look at adding any more plugins, that would seem to make sense. Have you any pointers in how best to approach this? No pointer at all. I have it my my head. And it's about time to pass this experience before I forget it =) The thing is I did it once and it took me ~ 2 months. Here is the brief plan that you need to keep: [1] Start with the lower lever. It's glib. Linux doesn't need any glib update - we use the system glib. So glib update is necessary for Win/Mac. [1.1] Latest glib has plenty of dependencies on other 3-rd party libraries. But there is one that's mandatory - libFFI. Fortunately Oracle has approval to use libFFI in it's products. But this probably isn't necessary for OpenJFX. So at you first step you need to bring libFFI sources, place them in rt/modules/media/src/main/native/gstreamer/3rd_party/libffi and make sure it builds on Windows and Mac producing lib/a for static linking. You can probably make dll/dylib instead but I don't think it's necessary. [1.2] Take the latest glib 2.38 or even 2.40. Place them in rt/modules/media/src/main/native/gstreamer/3rd_party/glib Note that you don't need all sources/headers. But to remove precisely what's redundant you should first compile gstreamer/plugins. Here you make sure you can compile and build glib-lite.dll/libglib-lite.dylib Having done 1.2 you should be able to run media component with the new glib-lite.dll. If it runs then you're done with glib upgrade. It's important to apply fixes that we made in glib to the newer glib library. You can find them by grepping for GSTREAMER_LITE in sources/headers. [2] GStreamer update. [3] Oracle plugins compilation/update. This step will also be necessary because 0.10.35 API is different from 1.0. For Example GstBuffer that we extensively use has incompatible APIs. I won't get deeper into details for [2] and [3] now. Let's just handle [1] and then continue.
Monadic operations on ObservableValue
EasyBind [1] now adds monadic operations on ObservableValue: http://tomasmikula.github.io/blog/2014/03/26/monadic-operations-on-observablevalue.html [1] http://www.fxmisc.org/easybind/
Re: Glass Robot and getSCreenCapture
Hi David,
Sorry to be getting to this so late. An uninitialized pixel is normally
considered to be black. If you throw an exception, clients will need to
either catch the exception or perform the same test that you are
performing before they call the API. Since this is not pubic API, no
client will be affected so even if we make the change to throw the
exception and then decide not to do this later, we can change it.
What is happening now? Who is being affected by this bug? Is it easy
to change the implementations to return black? This seems better to me
than throwing the exception.
As I say, if we throw the exception, then we will only break ourselves,
not clients of FX API. Have we ensured that the exception will not
break SQE tests.
Again, sorry to be getting to this so late and apologies if all of this
has been discussed in another thread that I missed,
Steve
On 2014-03-25 2:46 PM, David Hill wrote:
On 3/21/14, Mar 21, 12:53 PM, David Hill wrote:
Having heard a little feedback, here is my proposal in the form of a
review request :-)
My recommendation would be modify the JavaDoc contract to specify
that the x,y and x+width, y+height must be within the screen bounds,
with an IllegalArgumentException if out of bounds. This would be
checked in Robot, prior to calling the native impls.
This code is internal API, so I expect the real impact would be on
SQE.
I really like the idea of adding a bounds restriction - so that the
requested bounds must be within the Screen.
It seems the simplest solution to my issue of handling the odd edge
case of out of bound pixels, with the least likely impact.
This means that existing code in the implementations are not affected.
I suspect that there will we little if any impact on SQE tests, given
that most of us would avoid asking for a screen capture with undefined
pixels. I do expect that we will encounter a few exceptions to this
when tests are run on smaller displays (like embedded).
I also added bounds checking to Robot.getPixelColor() for consistency,
and because Embedded passes this call through to common code for
screen capture.
I did a grep through the JavaFX code base, and don't see any JavaFX
use cases. I expect any golden image test code could be affected.
I separated out this internal API changes from my embedded changes so
we have a clear and easy thing to review.
Jira: https://javafx-jira.kenai.com/browse/RT-36382
Patch: is inline in the Jira, but also here:
diff -r bb72bd2fa889
modules/graphics/src/main/java/com/sun/glass/ui/Robot.java
--- a/modules/graphics/src/main/java/com/sun/glass/ui/Robot.java Tue
Mar 25 14:21:26 2014 -0400
+++ b/modules/graphics/src/main/java/com/sun/glass/ui/Robot.java Tue
Mar 25 14:41:37 2014 -0400
@@ -144,9 +144,20 @@
protected abstract int _getPixelColor(int x, int y);
/**
* Returns pixel color at specified screen coordinates in IntARGB
format.
+ *
+ * If the requested pixel is not contained by the actual Screen
+ * bounds an IllegalArgumentException will be thrown.
+ *
+ * @param x The screen X of the requested capture (must be =0)
+ * @param y The screen Y of the requested capture (must be =0)
*/
public int getPixelColor(int x, int y) {
Application.checkEventThread();
+Screen s = Screen.getMainScreen();
+if (x 0 || y 0 ||
+x = s.getWidth() || y s.getHeight()) {
+ throw new IllegalArgumentException(Capture out of bounds);
+}
return _getPixelColor(x, y);
}
@@ -162,13 +173,27 @@
* will result in a Pixels object with dimensions (20x20).
Calling code
* should use the returned objects's getWidth() and getHeight()
methods
* to determine the image size.
- *
+ *
* If (@code isHiDPI) is {@code false}, the returned Pixels
object is of
* the requested size. Note that in this case the image may be
scaled in
* order to fit to the requested dimensions if running on a HiDPI
display.
+ *
+ * If the requested capture bounds is not contained by the actual
Screen
+ * bounds an IllegalArgumentException will be thrown.
+ *
+ * @param x The screen X of the requested capture (must be =0)
+ * @param y The screen Y of the requested capture (must be =0)
+ * @param width The of width the requested capture (must be =1
and fit on the screen)
+ * @param height The of width the requested capture (must be =1
and fit on the screen)
+ * @param isHiDPI return HiDPI if available
*/
public Pixels getScreenCapture(int x, int y, int width, int
height, boolean isHiDPI) {
Application.checkEventThread();
+Screen s = Screen.getMainScreen();
+if (x 0 || y 0 ||
+x + width s.getWidth() || y + height s.getHeight()) {
+ throw new IllegalArgumentException(Capture out of bounds);
+}
return _getScreenCapture(x, y, width,
Re: Glass Robot and getSCreenCapture
It isn't public API now.. but keep in mind that making it public in
some way has been requested for over two years. See RT-17571
Since those issues have made the internal Robot API known, no doubt
people are using it. (We all know how well people listen to the
warnings about using internal APIs .. *cough* sun.misc.Unsafe *cough*)
Cheers,
Scott
On Wed, Mar 26, 2014 at 10:59 AM, Stephen F Northover
steve.x.northo...@oracle.com wrote:
Hi David,
Sorry to be getting to this so late. An uninitialized pixel is normally
considered to be black. If you throw an exception, clients will need to
either catch the exception or perform the same test that you are performing
before they call the API. Since this is not pubic API, no client will be
affected so even if we make the change to throw the exception and then
decide not to do this later, we can change it.
What is happening now? Who is being affected by this bug? Is it easy to
change the implementations to return black? This seems better to me than
throwing the exception.
As I say, if we throw the exception, then we will only break ourselves, not
clients of FX API. Have we ensured that the exception will not break SQE
tests.
Again, sorry to be getting to this so late and apologies if all of this has
been discussed in another thread that I missed,
Steve
On 2014-03-25 2:46 PM, David Hill wrote:
On 3/21/14, Mar 21, 12:53 PM, David Hill wrote:
Having heard a little feedback, here is my proposal in the form of a
review request :-)
My recommendation would be modify the JavaDoc contract to specify that
the x,y and x+width, y+height must be within the screen bounds, with an
IllegalArgumentException if out of bounds. This would be checked in Robot,
prior to calling the native impls.
This code is internal API, so I expect the real impact would be on SQE.
I really like the idea of adding a bounds restriction - so that the
requested bounds must be within the Screen.
It seems the simplest solution to my issue of handling the odd edge case
of out of bound pixels, with the least likely impact.
This means that existing code in the implementations are not affected.
I suspect that there will we little if any impact on SQE tests, given that
most of us would avoid asking for a screen capture with undefined pixels. I
do expect that we will encounter a few exceptions to this when tests are run
on smaller displays (like embedded).
I also added bounds checking to Robot.getPixelColor() for consistency, and
because Embedded passes this call through to common code for screen capture.
I did a grep through the JavaFX code base, and don't see any JavaFX use
cases. I expect any golden image test code could be affected.
I separated out this internal API changes from my embedded changes so we
have a clear and easy thing to review.
Jira: https://javafx-jira.kenai.com/browse/RT-36382
Patch: is inline in the Jira, but also here:
diff -r bb72bd2fa889
modules/graphics/src/main/java/com/sun/glass/ui/Robot.java
--- a/modules/graphics/src/main/java/com/sun/glass/ui/Robot.java Tue Mar
25 14:21:26 2014 -0400
+++ b/modules/graphics/src/main/java/com/sun/glass/ui/Robot.java Tue Mar
25 14:41:37 2014 -0400
@@ -144,9 +144,20 @@
protected abstract int _getPixelColor(int x, int y);
/**
* Returns pixel color at specified screen coordinates in IntARGB
format.
+ *
+ * If the requested pixel is not contained by the actual Screen
+ * bounds an IllegalArgumentException will be thrown.
+ *
+ * @param x The screen X of the requested capture (must be =0)
+ * @param y The screen Y of the requested capture (must be =0)
*/
public int getPixelColor(int x, int y) {
Application.checkEventThread();
+Screen s = Screen.getMainScreen();
+if (x 0 || y 0 ||
+x = s.getWidth() || y s.getHeight()) {
+ throw new IllegalArgumentException(Capture out of bounds);
+}
return _getPixelColor(x, y);
}
@@ -162,13 +173,27 @@
* will result in a Pixels object with dimensions (20x20). Calling
code
* should use the returned objects's getWidth() and getHeight()
methods
* to determine the image size.
- *
+ *
* If (@code isHiDPI) is {@code false}, the returned Pixels object is
of
* the requested size. Note that in this case the image may be scaled
in
* order to fit to the requested dimensions if running on a HiDPI
display.
+ *
+ * If the requested capture bounds is not contained by the actual
Screen
+ * bounds an IllegalArgumentException will be thrown.
+ *
+ * @param x The screen X of the requested capture (must be =0)
+ * @param y The screen Y of the requested capture (must be =0)
+ * @param width The of width the requested capture (must be =1 and
fit on the screen)
+ * @param height The of width the requested capture (must be =1 and
Re: Glass Robot and getSCreenCapture
On 3/26/14, Mar 26, 10:59 AM, Stephen F Northover wrote:
Hi David,
Sorry to be getting to this so late. An uninitialized pixel is normally considered to be
black. If you throw an exception, clients will need to either catch the exception or
perform the same test that you are performing before they call the API. Since
this is not pubic API, no client will be affected so even if we make the change to throw
the exception and then decide not to do this later, we can change it.
What is happening now? Who is being affected by this bug? Is it easy to
change the implementations to return black? This seems better to me than
throwing the exception.
Why ? Because I happened to have to fix some code and saw that what we had in
ARM was broken, or at least made some poor assumptions around out of bounds
pixels.
I really, really don't like implied contract code, and robot screen capture
is certainly implied. My preference in javadoc, even on internal API that outlines the
expected behavior, instead of having to read impl code on platforms I am not familiar
with.
The main item still is:
* what does an out of bounds pixel look like. The current answer is -
depends. Certainly on ARM some of the existing code would return one of,
uninitialized, or 0. Note that 0 is not always the same as black (0xff00).
I sought to make the impl code easier by denying a call that would be out of
bounds. I am finding that this may be difficult or even impossible, when you
glue two screens of different sizes. Thanks to Anthony educating me on how this
works.
I did glance through the other impls code and I did not see any specific
handling of out of bounds pixels. Anthony says that handling was done by the
the underlying system calls.
As I say, if we throw the exception, then we will only break ourselves, not
clients of FX API. Have we ensured that the exception will not break SQE tests.
Ensured ? Not sure that is possible except by running the SQE tests with a
restriction in place. I have asked for a reading from SQE but have not heard
back yet.
But this discussion should be happening anyway, as we discuss what should or
should not be done.
Dave
Again, sorry to be getting to this so late and apologies if all of this has
been discussed in another thread that I missed,
Steve
On 2014-03-25 2:46 PM, David Hill wrote:
On 3/21/14, Mar 21, 12:53 PM, David Hill wrote:
Having heard a little feedback, here is my proposal in the form of a review
request :-)
My recommendation would be modify the JavaDoc contract to specify that the x,y
and x+width, y+height must be within the screen bounds, with an
IllegalArgumentException if out of bounds. This would be checked in Robot,
prior to calling the native impls.
This code is internal API, so I expect the real impact would be on SQE.
I really like the idea of adding a bounds restriction - so that the requested
bounds must be within the Screen.
It seems the simplest solution to my issue of handling the odd edge case of out
of bound pixels, with the least likely impact.
This means that existing code in the implementations are not affected.
I suspect that there will we little if any impact on SQE tests, given that most
of us would avoid asking for a screen capture with undefined pixels. I do
expect that we will encounter a few exceptions to this when tests are run on
smaller displays (like embedded).
I also added bounds checking to Robot.getPixelColor() for consistency, and
because Embedded passes this call through to common code for screen capture.
I did a grep through the JavaFX code base, and don't see any JavaFX use cases. I expect
any golden image test code could be affected.
I separated out this internal API changes from my embedded changes so we have a
clear and easy thing to review.
Jira: https://javafx-jira.kenai.com/browse/RT-36382
Patch: is inline in the Jira, but also here:
diff -r bb72bd2fa889 modules/graphics/src/main/java/com/sun/glass/ui/Robot.java
--- a/modules/graphics/src/main/java/com/sun/glass/ui/Robot.java Tue Mar 25
14:21:26 2014 -0400
+++ b/modules/graphics/src/main/java/com/sun/glass/ui/Robot.java Tue Mar 25
14:41:37 2014 -0400
@@ -144,9 +144,20 @@
protected abstract int _getPixelColor(int x, int y);
/**
* Returns pixel color at specified screen coordinates in IntARGB format.
+ *
+ * If the requested pixel is not contained by the actual Screen
+ * bounds an IllegalArgumentException will be thrown.
+ *
+ * @param x The screen X of the requested capture (must be =0)
+ * @param y The screen Y of the requested capture (must be =0)
*/
public int getPixelColor(int x, int y) {
Application.checkEventThread();
+Screen s = Screen.getMainScreen();
+if (x 0 || y 0 ||
+x = s.getWidth() || y s.getHeight()) {
+ throw new IllegalArgumentException(Capture out of bounds);
+}
return _getPixelColor(x, y);
}
[8u6] Review request: RT-36387: [IMX, Lens] Black rectangle seen instead of cursor on first run after reboot
Daniel, Rafi, Please review the simple fix for https://javafx-jira.kenai.com/browse/RT-36387 - [IMX, Lens] Black rectangle seen instead of cursor on first run after reboot Diff is in the jira. Thanks, Lisa
hg: openjfx/8u-dev/rt: RT-31075: Date picker fails with LocalDate.MAX and LocalDate.MIN values.
Changeset: 0f1a42edda54 Author:leifs Date: 2014-03-26 10:11 -0700 URL: http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/0f1a42edda54 RT-31075: Date picker fails with LocalDate.MAX and LocalDate.MIN values. ! modules/controls/src/main/java/com/sun/javafx/scene/control/skin/DatePickerContent.java
hg: openjfx/8u-dev/rt: 2 new changesets
Changeset: 91fdf1709eae Author:hudson Date: 2014-03-26 11:23 -0700 URL: http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/91fdf1709eae Added tag 8u20-b07 for changeset eee373287ad8 ! .hgtags Changeset: 5f8012b58540 Author:kcr Date: 2014-03-26 11:35 -0700 URL: http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/5f8012b58540 Automated merge with ssh://jfxsrc.us.oracle.com//javafx/8u/master/jfx/rt - apps/samples/Ensemble8/src/samples/java/ensemble/samples/language/swing/ProcessListener.java
[8u Review Request] RT-35864: Button's events are not working in a ListView
Jonathan, Please review: http://cr.openjdk.java.net/~dgrieve/RT-35864/webrev.00 A summary of the changes is in: https://javafx-jira.kenai.com/browse/RT-35864
Re: Glass Robot and getSCreenCapture
On 3/26/14, Mar 26, 12:53 PM, Stephen F Northover wrote: I don't like implied contract code either but I also don't like exceptions for cases like this. I would prefer that we return zero for pixels that are unspecified as this seems better than testing screen bounds (which can get you in trouble on multi-display monitors). Anyway, to fix this involves writing a test case that we can run on all platforms in a multi-monitor scenario. Also, the primary monitor can be on either the left or the right and this might affect the result. It's easier to fix this in Java code by testing screen bounds as you were doing and throw the exception. Since this is not API and we need to move on, then we could do this (and possibly break SQE). Alternately, we can construct the test cases, see if the platforms/glass already return zero and assess where to go next. Whatever happens, we need a test case. Is there one in the JIRA? If so, I can run it on the desktop platforms and let you know the results for the current code base. As much as I don't like it - I am no longer sure there is a reasonable pre-test that can be done. I suggested a four corner test, but in the case of two adjacent screens of different heights, it is reasonable to see that you could ask for bounds that would put 3 corners in, and one out (hopefully the asci art will work): +++ ||| ||| ||| ++| || ++ For a one shot screen capture - we would pass in top left and width and height to make the bottom right. Currently this should work on Mac I am told, though what is in the out of bounds pixels is not known. And if we added a third tall screen to the left, life gets even more complicated :-( I was hoping to simplify the native impl for ARM by making it impossible to have an out of bounds pixel. This thought was in line with other API - check for valid values before calling the impl. We still could, but in the above case, there would not be a way to get all the screen in one shot. I really don't think we should be having a major impact on SQE, as I would think that most golden image tests will be based on checking known things - like the content of a window. But ... I have erred recently in the past on this subject... :-) The test case I have been using is in HelloSanity. It is well behaved. It is only one of 2 apps in our repo that perform any screen captures (an the other was used as the basis for HelloSanity). There are some uses of getPixel(x,y) which is a variation. I found the problem in the ARM code by inspection, and have yet to write a reasonable test app that includes the aprox 6-8 variations of overlap (ie, full subset, off left, off right, completely missing up, .) I certainly can throw together something that will try some of the variations to see if we fail on other platforms. Given my current understanding of the problem though, I really don't see how a pre-verification of the bounds can be done. -- David Hill david.h...@oracle.com Java Embedded Development Experience is a hard teacher because she gives the test first, the lesson afterwards. -- Vernon Sanders Law
Re: Glass Robot and getSCreenCapture
For a one shot screen capture - we would pass in top left and width and height to make the bottom right. Currently this should work on Mac I am told, though what is in the out of bounds pixels is not known. And if we added a third tall screen to the left, life gets even more complicated :-( When pressing Command+Shift+3, the Mac creates separate images for each display. -DrD-
hg: openjfx/2u/dev/rt: Added tag 2.2.60-b12 for changeset c1541c2d6a96
Changeset: ad9fa39517da Author:hudson Date: 2014-03-26 08:12 -0700 URL: http://hg.openjdk.java.net/openjfx/2u/dev/rt/rev/ad9fa39517da Added tag 2.2.60-b12 for changeset c1541c2d6a96 ! .hgtags
hg: openjfx/8u-dev/rt: 2 new changesets
Changeset: bd741e03a5d2 Author:jgiles Date: 2014-03-27 09:16 +1300 URL: http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/bd741e03a5d2 RT-36392: [SplitPane] Cannot lookup children inside a SplitPane ! modules/controls/src/main/java/com/sun/javafx/scene/control/skin/SplitPaneSkin.java ! modules/controls/src/test/java/javafx/scene/control/SplitPaneTest.java Changeset: cf8541657afc Author:jgiles Date: 2014-03-27 09:17 +1300 URL: http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/cf8541657afc RT-36316: [Accessibility] Improve TreeView virtualization on Windows ! modules/controls/src/main/java/com/sun/javafx/scene/control/skin/TreeViewSkin.java ! modules/controls/src/main/java/com/sun/javafx/scene/control/skin/VirtualFlow.java
hg: openjfx/8u-dev/rt: 2 new changesets
Changeset: 335ad99f854f Author:kcr Date: 2014-03-26 14:40 -0700 URL: http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/335ad99f854f RT-35019: [3D] NPE in NGShape.renderContent when drawing empty shapes Reviewed-by: flar ! modules/graphics/src/main/java/com/sun/javafx/sg/prism/NGShape.java + tests/system/src/test/java/test3d/RT35019Test.java Changeset: 755b1d48c070 Author:kcr Date: 2014-03-26 14:40 -0700 URL: http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/755b1d48c070 [TEST-ONLY] RT-31190: RT_5239Test uses java.awt.Color instead of Prism color ! modules/graphics/src/test/java/com/sun/scenario/effect/rt_5239/RT_5239Test.java
Re: Glass Robot and getSCreenCapture
Hi David, sorry to keep you waiting with an answer from Embedded SQE. I assume there will be no major impact on SQE since as you told and it is true, most golden image tests we have are based on a window content. Anyway from what I understood your implementation will be easily rolled back if we reveal some unexpected impact on tests, right? 26.03.2014 23:03, David Hill ?: On 3/26/14, Mar 26, 12:53 PM, Stephen F Northover wrote: I don't like implied contract code either but I also don't like exceptions for cases like this. I would prefer that we return zero for pixels that are unspecified as this seems better than testing screen bounds (which can get you in trouble on multi-display monitors). Anyway, to fix this involves writing a test case that we can run on all platforms in a multi-monitor scenario. Also, the primary monitor can be on either the left or the right and this might affect the result. It's easier to fix this in Java code by testing screen bounds as you were doing and throw the exception. Since this is not API and we need to move on, then we could do this (and possibly break SQE). Alternately, we can construct the test cases, see if the platforms/glass already return zero and assess where to go next. Whatever happens, we need a test case. Is there one in the JIRA? If so, I can run it on the desktop platforms and let you know the results for the current code base. As much as I don't like it - I am no longer sure there is a reasonable pre-test that can be done. I suggested a four corner test, but in the case of two adjacent screens of different heights, it is reasonable to see that you could ask for bounds that would put 3 corners in, and one out (hopefully the asci art will work): +++ ||| ||| ||| ++| || ++ For a one shot screen capture - we would pass in top left and width and height to make the bottom right. Currently this should work on Mac I am told, though what is in the out of bounds pixels is not known. And if we added a third tall screen to the left, life gets even more complicated :-( I was hoping to simplify the native impl for ARM by making it impossible to have an out of bounds pixel. This thought was in line with other API - check for valid values before calling the impl. We still could, but in the above case, there would not be a way to get all the screen in one shot. I really don't think we should be having a major impact on SQE, as I would think that most golden image tests will be based on checking known things - like the content of a window. But ... I have erred recently in the past on this subject... :-) The test case I have been using is in HelloSanity. It is well behaved. It is only one of 2 apps in our repo that perform any screen captures (an the other was used as the basis for HelloSanity). There are some uses of getPixel(x,y) which is a variation. I found the problem in the ARM code by inspection, and have yet to write a reasonable test app that includes the aprox 6-8 variations of overlap (ie, full subset, off left, off right, completely missing up, .) I certainly can throw together something that will try some of the variations to see if we fail on other platforms. Given my current understanding of the problem though, I really don't see how a pre-verification of the bounds can be done. -- Best regards, Stas Smirnov Stas Smirnov | Java Embedded Phone: +7 812 3346130 | Mobile: +7 921 9262241 Oracle Development SPB, LLC 10th Krasnoarmeyskaya 22A, St. Petersburg, 190103, Russia
