• Type: Enhancement
• Component: javafx
• Sub-Component: graphics
• Affected Version: 8

• Priority: P3
• Status: Resolved
• Resolution: Fixed

• Submitted: 2012-11-07
• Updated: 2015-06-12
• Resolved: 2014-09-05

JDK 8
8u40Fixed

Changeset: a56acb9d85fa Author: Chien Yang <chien.yang@oracle.com> Date: 2014-09-04 20:50 -0700 URL: http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/a56acb9d85fa 05-09-2014 The only reference I saw was in the "common mistakes" URL that I quoted above... 05-09-2014 Yes, you are correct. I just verified that we always make 2 calls. BTW, I can't find document specifying the need to call GL_GENERATE_MIPMAP for sub texture update, but Kevin confirmed it only needed for texture creation. This also matches the result of the test program included in the webrev. So I think we are good. 04-09-2014 For #6, note that in ES2Texture.java all of the calls to texImage2D either pass null or an empty buffer that was just allocated a few lines previously. The pixels are uploaded later in the method in the "if (pixels != null)" section. So, there are always 2 calls as far as I can see. 04-09-2014 #5. Thanks I will fix SWResourceFactory locally before I push. #6. It is done in one call to texImage2D() at first creation (when create is true). It will write a test program to verify texSubImage2D doesn't need the GL_GENERATE_MIPMAP setting. I will file a new JIRA on texture update if the setting is required. It will likely be true for d3d as well if it is needed. 04-09-2014 You forgot #5 for SWResourceFactory. For #6 I guess I was thinking that texImage2D was an uploading operation, but I see that it also creates. It can do both, but it looks like we are using it only for creation and we always use texSubImage2D for actually loading pixels (a bit of a waste I guess since we could get it done in one call to texImage2D, no?) So, fixing SWResourceFactory looks like this will be done... 04-09-2014 Hi Jim, Thanks for offering the extra pair of eyes. http://cr.openjdk.java.net/~ckyang/RT-26108/webrev.03/ 1) Fixed. I missed this one. 2) Fixed. 3) I agree, but it also has its downside to teach mipmap to TextureResourcePool interface. I would have touch the 5 implementing classes and duplicating the looping logic in at least es2 and d3d. Hence I decided to keep its logic in just BaseResourceFactory. 4) Fixed. It is a leftover I forgot to delete. 5) Fixed. It was inserted by Netbeans that I missed in fixing. J2DResourceFactory shouldn't care about mipmap, it should call create() without the mipmap flag. 6) Yes, we call it once on texture generation. Our minimum supported OpenGL version is at 2.1 hence we have to use the legacy approach. glGenerateMipmap() requires OpenGL 3.0 or greater and it can be a challenge on the embedded platform. 04-09-2014 ES2Texture/750: false -> useMipMap for the repeatL case? I just realized there is a bug in my mipmap size code. It should be a regular while loop, not a do/while and after the loop there should be a size += 1. Otherwise we never account for that last 1x1 and if the input dimensions are 1x1 then we say the size is 0. BaseResourceFactory/198: maybe TextureResourcePool should have a mipmap boolean on its estimateTextureSize so that we don't have 2 different classes doing the calculations for the 2 different cases. Also, if there is ever a case where some implementation has a quirky way of allocating mipmaps then we can customize the algorithm for that single pool class. BaseResourceFactory/212: This TODO belongs in the Texture object implementations, but I think you've already updated both ES2 and D3D to deal with this anyway? If not, then you need to have a comment with a Jira reference in the place that is missing something (this particular location doesn't need to deal with isMipMap since it is calling a method on a texture object which should now know whether it is mipmap or not due to the flag). J2DResourceFactory/120: make the exception contingent on useMipMap=true in case someone calls this factory method with "false", or simply ignore it? (Is the flag binding, or just a hint?) SWResourceFactory/175: same thing // http://www.opengl.org/wiki/Common_Mistakes GLContext.c: apparently GL_GENERATE_MIPMAP is part of the texture state so we really only need to set it once...? However it is also deprecated as of OGL3.0 and gone in 3.1... 04-09-2014 http://cr.openjdk.java.net/~ckyang/RT-26108/webrev.02/ 1) Add mipmap flag to BaseTexture 2) Use looping to estimate total memory used for texture with mipmap 3) Push up texture min and max params setting logic to Java side 04-09-2014 I completely agree with Jim's point regarding logic in the native methods. I also think that using the logic Jim suggests for choosing between LML and NMN is the easiest way to gain consistency between D3D and OGL without introducing more internal API. 03-09-2014 These are excellent feedback! I will include them in the next webrev. 03-09-2014 Also, in general, I prefer to see the native methods have little "API" logic in them at least as far as these parameter setting methods. The Java code should have full control over the values used rather than pass in some "indicators" that get processed into the necessary values. That means less complexity in the native methods, more control from Java should we need to coordinate the logic with other data that we grab from other locations, and less need to revisit the native methods in the future. We already echo the GL_LINEAR and GL_NEAREST values in Java, we should probably add the GL_*_MIPMAP_* values and pass down the values we want to set... 03-09-2014 I don't have a strong opinion on mixing N_M_L and L_M_N since I have no idea why anyone would want to use those two modes. How are the various mipmap levels created in the automatic creation? As LINEAR interpolations from the base? If so, then I'm not sure why you would want anything but L_M_L for mipmapping...? For consistency and minimal perturbations from the existing code we could simply do what D3D does with: setTexParam(MAG, param); if (mipmap) { param = (param == LINEAR) ? LML : N_M_N; } setTexParam(MIN, param); 03-09-2014 We should be able to have the same behavior on both pipeline, depending on what we want to have happen. The question is whether or not we should interpolate between the two closest mipmap levels if mipmapping is present, even when doing nearest neighber sampling on the image itself. For OpenGL this is the difference between NEAREST_MIPMAP_LINEAR and NEAREST_MIPMAP_NEAREST. For D3D it it the difference between setting the mip parameter to linear if mipmapping is enabled, or setting it to the same value as the min filter when mipmapping is enabled. 02-09-2014 For D3D we specify the same interpolation for all of min/max/mip - either POINT or LINEAR and it is based on what they asked for min/max. For ES2 we have 4 cases (1 or 2 of which probably never happens): (interp, mipmap => min/max/mipmap) NEAREST, false => N/N/not specified NEAREST, true => L/N/L LINEAR, false => L/L/not specified LINEAR, true => L/L/L I don't think we currently have any cases that ask for NEAREST anyway, but I may fix that soon. The main differences are that ES2 doesn't specify any mipmap behavior unless it is specifically requested, but D3D always sets the value. That is probably OK since the value would get ignored unless we have a mipmap so we only have the loose association of whether a mipmap is specified and what params are handed to the native method to make sure that gets set up right. The other difference is that in the case of asking for NEAREST with mipmap (which I don't think we can currently specify) gives us N/N/N with D3D and L/N/L with ES2...? 02-09-2014 For the error, "Mipmap not supported with CLAMP mode"? 02-09-2014 Eek, Kevin is right about the sizes. Given that we only calculate this on creation, it might not hurt to just do it with a loop anyway (make a helper function): static long sizeWithMipMap(int w, int h, int bytesPerPixel) { long size = 0; do { size += ((long) w) * ((long) h); w = (w + 1) >> 1; h = (h + 1) >> 1; } while (w > 1 && h > 1); return size * bytesPerPixel; } Do we need to deal with scan strides there? The dimensions should be the physW and physH in the first place. 02-09-2014 * I'll let Jim comment about tagging the texture with a mipmap flag, but it seems like a reasonable idea to me. It would mean that updating an existing texture (the Texture.update methods) would know whether or not a mipmap texture is needed. This would be important if you sometimes need to recreate the texture from the update method. * Following on to another of Jim's points. Rather than hard-coding LINEAR_MIPMAP_LINEAR in the native code, would it be better to test pname and use LINEAR_MIPMAP_LINEAR or NEAREST_MIPMAP_NEAREST depending on its value? If not, at least a comment to that effect is in order. A couple additional points. 1. Concerning the expansion factor for mipmaps. The 1/3 increase in texture memory is only accurate for square textures. When mipmaping a non-square texture, the smaller dimension will be clamped at 1 for the last "N" levels, where N = log2(max(w,h)) - log2(min(w,h)). In the worst case, for an Nx1 texture you need almost 2X the amount of memory. If this matters then we might want to do something like: if (useMipmap) { if (w == h) { // Mipmap uses 1/3 more memory size = (int) Math.ceil(size * 1.34); } else { // loop through and compute the multiplier } } 2. Minor point. The following is not quite grammatically correct: throw new IllegalArgumentException("Doesn't useMipmap to mix with CLAMP mode: useMipmap = " Maybe better to say; "Cannot mix useMipMap with CLAMP mode" 02-09-2014 Here is the revised webev: http://cr.openjdk.java.net/~ckyang/RT-26108/webrev.01/ I have included the 2 suggestions from Jim and improved the test program texture generation code. 1) Switch mipmap memory usage estimate from 1.5X to 1.34X 2) Throws IllegalArgumentException if useMipmap is used for CLAMP mode. I decided not to do the work to add mipmap tag to texture as is work is unlikely to save/clean the code much at this point. We still need to pass down useMipmap at texture creation to auto generate mipmap. 02-09-2014 Also, for #1 my comment was not "is the current code correct" because I can see that. My comment was that it should be an error to request mipmapping for the other wrap type because it will be silently ignored if we ever do that which leads to silent bugs that are hard to track down. If we know that CLAMP images cannot be (currently) mipmapped then it should not allow you to create (a broken) one... 29-08-2014 It's 1 and 1/3 so 1.3x would be too small. Unfortunately 1 1/3 is a repeating decimal so 1.34 just to be safe? For #3 and #4 I'd like to have mipmap at least tagged on the texture as a property so that we know which constant to use when we fix this later. Also, this may remove the need to have the upload methods take an extra parameter if the texture object is tagged...? 29-08-2014 Hi Jim, 1) Yes, all textures used for 3D are hard coded to REPEAT. Please see ES2PhongMaterial/85 (similar setting is done for d3d) 2) Yes, I know it will be more than 1.25X and less than 1.5X. Hence I picked to upper bound for 1.5X without doing the exact math. I will drop it to 1.3X if you are okay. 3) and 4) Agreed. This is a fragile system on the es2 side (a "JOGLism" that we left behind) , d3d is much cleaner and supports NEAREST and LINEAR. We should look into refactoring this code separating as standalone task. What do you think? 29-08-2014 Some comments: - BaseResourceFactory/141 - Are mipmap textures always requested with REPEAT? Should it throw an exception in the CLAMP case if mipmap is true so we don't get problems? - BaseResourceFactory/184 - Originally I thought they took 2x the memory, but then I realized that the sizes go down by 4x on each level. There is an article that says it only takes 33% more and demonstrates it with RGB images in a grid, but I think the same thing applies to ARGB images (if you sum 1/4^n you approach 1/3 as a result). Note that calling setMinMax is fragile, because the filter state is updated every time an image is used - see ES2Context.updateTexture() which uses 2 helper functions to update both the WRAP and FILTER values in OpenGL based on what it thinks the value used to be. Also, I eventually want to make linear filtering enabled on a case by case basis so that we can implement ImageView.setSmooth(). This new mechanism should not be fragile in the face of that eventual work. Right now at least all images are assumed LINEAR, but with this work you are doing, some of them should be assumed to be mipmapped and so we need to know whether to use GL_NEAREST or GL_*_MIPMAP_NEAREST when that change comes down the pipe. Perhaps the calls to setMinMax when we create a texture should be removed in favor of a more robust updateTexture()->updateFilter() sequence? I haven't finished reviewing all of the files, but I wanted to get these comments out for now... 29-08-2014 Hi Kevin and Jim, Please review the proposed implementation. A test program, CheckerMeshViewer.java, is also included in this webrev: http://cr.openjdk.java.net/~ckyang/RT-26108/webrev.00/ 29-08-2014 Reschedule for Van Ness as it is too late for Lombard. 06-08-2013