Hi Team, this is regarding: - vo_gl - HD video material - AMD's low performance glTexSubImage2D() texture upload. I have experienced a very low texture upload performance with AMD GPU's while _trying_ to watch HD content. A little performance test using valgrind, showed my the it's glTexSubImage2D() (again). It looks like AMD still cannot do DMA transfers automatically, sad thing. Therefor I have added a 'pbodma' option, to use PBO's for texture uploads. It looks silly, since we do an extra dma xfer with memcpy first, and then let glTexSubImage2D() doing a DMA using PBO, but it's still faster for AMD GPU's. On a AMD 780G motherboard IPG HD3xxx we will gain a performance hit of 82% -> 24%, which makes it usable. Little performance hit on NVidia 3% -> 2%, but still :) Sad thing for AMD (again), they cannot really handle GL_UNPACK_ROW_LENGTH, therefor the only working combination for my AMD 780G is: 'rectangle=0:pbodma=1' However, it renders vo gl usable for this combination. Please submit and/or let me know your concerns. Cheers, Sven -- health & wealth mailto:sgothel@jausoft.com ; www : http://www.jausoft.ca ; pgp: http://www.jausoft.com/gpg/ land : +1 (780) 637 3842 ; cell: +1 (780) 952 4481 Timezone MST: EST-2, UTC-7, CET-8 ; MDT: EDT-2, UTC-6, CEDT-8
just a clean patch against the latest svn .. (no changes though, just the en/mplayer.1 line number differs) tested ok. ~Sven On Monday 28 April 2008 03:57:56 Sven Gothel wrote:
Hi Team,
this is regarding: - vo_gl - HD video material - AMD's low performance glTexSubImage2D() texture upload.
I have experienced a very low texture upload performance with AMD GPU's while _trying_ to watch HD content.
A little performance test using valgrind, showed my the it's glTexSubImage2D() (again).
It looks like AMD still cannot do DMA transfers automatically, sad thing.
Therefor I have added a 'pbodma' option, to use PBO's for texture uploads.
It looks silly, since we do an extra dma xfer with memcpy first, and then let glTexSubImage2D() doing a DMA using PBO, but it's still faster for AMD GPU's.
On a AMD 780G motherboard IPG HD3xxx we will gain a performance hit of 82% -> 24%, which makes it usable.
Little performance hit on NVidia 3% -> 2%, but still :)
Sad thing for AMD (again), they cannot really handle GL_UNPACK_ROW_LENGTH, therefor the only working combination for my AMD 780G is: 'rectangle=0:pbodma=1' However, it renders vo gl usable for this combination.
Please submit and/or let me know your concerns.
Cheers, Sven
-- health & wealth mailto:sgothel@jausoft.com ; www : http://www.jausoft.ca ; pgp: http://www.jausoft.com/gpg/ land : +1 (780) 637 3842 ; cell: +1 (780) 952 4481 Timezone MST: EST-2, UTC-7, CET-8 ; MDT: EDT-2, UTC-6, CEDT-8
On Monday 28 April 2008 03:57:56 Sven Gothel wrote:
Hi Team,
fixing a minor possible bug in case bytes-per-pixel!=1 in gl_common.c btw .. would anybody like to review this ? thank you. ~sven
this is regarding: - vo_gl - HD video material - AMD's low performance glTexSubImage2D() texture upload.
I have experienced a very low texture upload performance with AMD GPU's while _trying_ to watch HD content.
A little performance test using valgrind, showed my the it's glTexSubImage2D() (again).
It looks like AMD still cannot do DMA transfers automatically, sad thing.
Therefor I have added a 'pbodma' option, to use PBO's for texture uploads.
It looks silly, since we do an extra dma xfer with memcpy first, and then let glTexSubImage2D() doing a DMA using PBO, but it's still faster for AMD GPU's.
On a AMD 780G motherboard IPG HD3xxx we will gain a performance hit of 82% -> 24%, which makes it usable.
Little performance hit on NVidia 3% -> 2%, but still :)
Sad thing for AMD (again), they cannot really handle GL_UNPACK_ROW_LENGTH, therefor the only working combination for my AMD 780G is: 'rectangle=0:pbodma=1' However, it renders vo gl usable for this combination.
Please submit and/or let me know your concerns.
Cheers, Sven
On Tue, Apr 29, 2008 at 11:51:20AM -0600, Sven Gothel wrote:
fixing a minor possible bug in case bytes-per-pixel!=1 in gl_common.c
btw .. would anybody like to review this ? thank you.
I have very little time right now, but I will look at it (this is not a complete review). I do quite dislike the code duplication in the draw_slice function though, and it is quite unclear which part of the changes make the biggest difference, e.g. if implementing MP_IMGTYPE_IP and MP_IMGTYPE_IPB in get_image would be enough. Also the patch changing mp_msg calls at the same time and converting existing code to use the gl...PBO wrapper functions (which so far seem a bit like overkill to me anyway) make this patch quite hard to understand. In addition, I have some doubts if using more than one PBO is a good idea.
@@ -646,16 +666,30 @@ static int draw_slice(uint8_t *src[], int stride[], int w,int h,int x,int y) { mpi_flipped = (stride[0] < 0); - glUploadTex(gl_target, gl_format, gl_type, src[0], stride[0], - x, y, w, h, slice_height); - if (image_format == IMGFMT_YV12) { - ActiveTexture(GL_TEXTURE1); - glUploadTex(gl_target, gl_format, gl_type, src[1], stride[1], - x / 2, y / 2, w / 2, h / 2, slice_height); - ActiveTexture(GL_TEXTURE2); - glUploadTex(gl_target, gl_format, gl_type, src[2], stride[2], - x / 2, y / 2, w / 2, h / 2, slice_height); - ActiveTexture(GL_TEXTURE0); + if(!use_pboDMA) { + glUploadTex(gl_target, gl_format, gl_type, src[0], stride[0], + x, y, w, h, slice_height); + if (image_format == IMGFMT_YV12) { + ActiveTexture(GL_TEXTURE1); + glUploadTex(gl_target, gl_format, gl_type, src[1], stride[1], + x / 2, y / 2, w / 2, h / 2, slice_height); + ActiveTexture(GL_TEXTURE2); + glUploadTex(gl_target, gl_format, gl_type, src[2], stride[2], + x / 2, y / 2, w / 2, h / 2, slice_height); + ActiveTexture(GL_TEXTURE0); + }
Reindentation (aka "cosmetics") may not be mixed with functional changes.
@@ -882,6 +949,17 @@ "Use -vo gl:nomanyfmts if playback fails.\n"); mp_msg (MSGT_VO, MSGL_V, "[gl] Using %d as slice height " "(0 means image height).\n", slice_height); + + switch(use_pboDMA) { + case 1: + gl_pboDMA = PBO_XFER_MULTIPLE_MEMCPY; + break; + case 2: + gl_pboDMA = PBO_XFER_SINGLE_MEMCPY; + break; + default: + gl_pboDMA = PBO_XFER_DISABLED; + }
Decoupling UI and internal code is a good idea, but it also bloats the code. I would prefer if the user would just set gl_pboDMA directly. If ever a proper separation is desired, it would be better to extend the parser to support it directly.
+ if ( pbo->mode==PBO_XFER_SINGLE_MEMCPY && h * stride > pbo->sz ) { + pbo->mode=PBO_XFER_MULTIPLE_MEMCPY; + mp_msg (MSGT_VO, MSGL_INFO, "[gl] glUploadTexPBO %d: PBO single -> multiple memcpy (video stride)!\n", + pbo->name);
If you intend to check if there is a stride applied, "h * stride > pbo->sz" is the wrong condition, "stride > w * bytes_per_pixel" is the right one.
+ if ( h * w > pbo->sz ) + { + mp_msg (MSGT_VO, MSGL_INFO, "[gl] glUploadTexPBO %d: %d/%d %dx%d, sz %d, slic %d, strd %d, bpp %d -> PBO Disabled (video size)!\n", + pbo->name, x,y, w,h, pbo->sz, slice, stride, bytesPerPixel); + glDestroyPBO(pbo); + + // fallback .. + glUploadTex(target, format, type, dataptr, stride, x, y, w, h, slice); + return; + }
Either this is possible, then it should be supported properly (not just via fallback) or it should just not do anything, according to the motto: better fail hard than continue with an unexpected performance degradation - users are unlikely to notice the message, failing will ensure we will get a bug report instead of a user just thinking "oh MPlayer is so slow..." and never reporting it.
+ if((pbo->test&1)==0) { + mp_msg (MSGT_VO, MSGL_V, "[gl] glUploadTexPBO %d: %d/%d %dx%d, sz %d, slic %d, strd %d, bpp %d - mode %d\n", + pbo->name, x,y, w,h, pbo->sz, slice, stride, bytesPerPixel, pbo->mode); + pbo->test|=1; + }
IMO that has no place in productive code.
+ if(pbo->mode==PBO_XFER_SINGLE_MEMCPY) { + memcpy(pbo->mem, data, h*stride); + rowlenBytes = stride; + } else if(pbo->mode==PBO_XFER_MULTIPLE_MEMCPY) { + for(i=0;i<h;i++) + { + memcpy(pbo->mem+(i*pbo->stride), data+(i*stride), w*bytesPerPixel); + }
You should probably look at fast_memcpy, mem2agpcpy, memcpy_pic and mem2agpcpy_pic Greetings, Reimar Döffinger
On Tuesday 29 April 2008 12:24:28 Reimar Döffinger wrote:
On Tue, Apr 29, 2008 at 11:51:20AM -0600, Sven Gothel wrote:
fixing a minor possible bug in case bytes-per-pixel!=1 in gl_common.c
btw .. would anybody like to review this ? thank you.
I have very little time right now, but I will look at it (this is not a complete review). I do quite dislike the code duplication in the draw_slice function though, it is not necessary, and the distiction can be don within the Upload function, which can be unified as well .. yeah right, I guess that's better.
and it is quite unclear which part of the changes make the biggest difference, e.g. if implementing MP_IMGTYPE_IP and MP_IMGTYPE_IPB in get_image would be enough.
btw .. the original PBO usage doesn't work, ie is against the PBO spec. get_image: - maps the PBO .. PBO mem content changed by video codec .. draw_image: - upload data to texture, glTexSubImage2D - unmap the PBO but it should be: - map memory - change data - unmap memory - data upload or am I wrong ? I have tested both, but only the latter works on other than NVidia. IMHO the original fast PBO path should be removed, well .. it doesn't hit anyway, since the requirements for get_image -> PBO, are never met (as much as I have tested it).
Also the patch changing mp_msg calls at the same time and converting existing code to use the gl...PBO wrapper functions (which so far seem a bit like overkill to me anyway) make this patch quite hard to understand.
I will unify the Upload functionality ..
In addition, I have some doubts if using more than one PBO is a good idea.
Well, it works and it is much faster. I.e. creating a buffer object is expensive, switching is cheap.
@@ -646,16 +666,30 @@ static int draw_slice(uint8_t *src[], int stride[], int w,int h,int x,int y) { mpi_flipped = (stride[0] < 0); - glUploadTex(gl_target, gl_format, gl_type, src[0], stride[0], - x, y, w, h, slice_height); - if (image_format == IMGFMT_YV12) { - ActiveTexture(GL_TEXTURE1); - glUploadTex(gl_target, gl_format, gl_type, src[1], stride[1], - x / 2, y / 2, w / 2, h / 2, slice_height); - ActiveTexture(GL_TEXTURE2); - glUploadTex(gl_target, gl_format, gl_type, src[2], stride[2], - x / 2, y / 2, w / 2, h / 2, slice_height); - ActiveTexture(GL_TEXTURE0); + if(!use_pboDMA) { + glUploadTex(gl_target, gl_format, gl_type, src[0], stride[0], + x, y, w, h, slice_height); + if (image_format == IMGFMT_YV12) { + ActiveTexture(GL_TEXTURE1); + glUploadTex(gl_target, gl_format, gl_type, src[1], stride[1], + x / 2, y / 2, w / 2, h / 2, slice_height); + ActiveTexture(GL_TEXTURE2); + glUploadTex(gl_target, gl_format, gl_type, src[2], stride[2], + x / 2, y / 2, w / 2, h / 2, slice_height); + ActiveTexture(GL_TEXTURE0); + }
Reindentation (aka "cosmetics") may not be mixed with functional changes.
Yeah .. come on :) Next time I will squash the white spaces from the review .. Seriously, the block shifted one right ..
@@ -882,6 +949,17 @@ "Use -vo gl:nomanyfmts if playback fails.\n"); mp_msg (MSGT_VO, MSGL_V, "[gl] Using %d as slice height " "(0 means image height).\n", slice_height); + + switch(use_pboDMA) { + case 1: + gl_pboDMA = PBO_XFER_MULTIPLE_MEMCPY; + break; + case 2: + gl_pboDMA = PBO_XFER_SINGLE_MEMCPY; + break; + default: + gl_pboDMA = PBO_XFER_DISABLED; + }
Decoupling UI and internal code is a good idea, but it also bloats the code. I would prefer if the user would just set gl_pboDMA directly. If ever a proper separation is desired, it would be better to extend the parser to support it directly.
I thought about it, ie casting int -> enum. Ok .. will do.
+ if ( pbo->mode==PBO_XFER_SINGLE_MEMCPY && h * stride > pbo->sz ) { + pbo->mode=PBO_XFER_MULTIPLE_MEMCPY; + mp_msg (MSGT_VO, MSGL_INFO, "[gl] glUploadTexPBO %d: PBO single -> multiple memcpy (video stride)!\n", + pbo->name);
If you intend to check if there is a stride applied, "h * stride > pbo->sz" is the wrong condition, "stride > w * bytes_per_pixel" is the right one.
No, here we check if a single memcpy would not exceed the texture bounds. For multiple memcpy, yes, you are right, and we do this later on ..
+ if ( h * w > pbo->sz ) + { + mp_msg (MSGT_VO, MSGL_INFO, "[gl] glUploadTexPBO %d: %d/%d %dx%d, sz %d, slic %d, strd %d, bpp %d -> PBO Disabled (video size)!\n", + pbo->name, x,y, w,h, pbo->sz, slice, stride, bytesPerPixel); + glDestroyPBO(pbo); + + // fallback .. + glUploadTex(target, format, type, dataptr, stride, x, y, w, h, slice); + return; + }
Either this is possible, then it should be supported properly (not just via fallback) or it should just not do anything, according to the motto: better fail hard than continue with an unexpected performance degradation - users are unlikely to notice the message, failing will ensure we will get a bug report instead of a user just thinking "oh MPlayer is so slow..." and never reporting it.
hmm .. as you wish, so let's fail hard :)
+ if((pbo->test&1)==0) { + mp_msg (MSGT_VO, MSGL_V, "[gl] glUploadTexPBO %d: %d/%d %dx%d, sz %d, slic %d, strd %d, bpp %d - mode %d\n", + pbo->name, x,y, w,h, pbo->sz, slice, stride, bytesPerPixel, pbo->mode); + pbo->test|=1; + }
IMO that has no place in productive code.
right :)
+ if(pbo->mode==PBO_XFER_SINGLE_MEMCPY) { + memcpy(pbo->mem, data, h*stride); + rowlenBytes = stride; + } else if(pbo->mode==PBO_XFER_MULTIPLE_MEMCPY) { + for(i=0;i<h;i++) + { + memcpy(pbo->mem+(i*pbo->stride), data+(i*stride), w*bytesPerPixel); + }
You should probably look at fast_memcpy, mem2agpcpy, memcpy_pic and mem2agpcpy_pic
I will look at these, thank you. However, the default memcpy today will make the right choice, ie: CPU unroled inline .., CPU function call, DMA xfer .. Thanks lot for your review, Reimar. Cheers, Sven
Greetings, Reimar Döffinger _______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
-- health & wealth mailto:sgothel@jausoft.com ; www : http://www.jausoft.ca ; pgp: http://www.jausoft.com/gpg/ land : +1 (780) 637 3842 ; cell: +1 (780) 952 4481 Timezone MST: EST-2, UTC-7, CET-8 ; MDT: EDT-2, UTC-6, CEDT-8
To do the unification, one more question .. Why is that orig Upload function so complicated ? I might miss something here completly, but IMHO: +++ glAdjustAlignment(stride); glPixelStorei(GL_UNPACK_ROW_LENGTH, stride / glFmt2bpp(format, type)); for (; y + slice <= y_max; y += slice) { glTexSubImage2D(target, 0, x, y, w, slice, format, type, data); data += stride * slice; } if (y < y_max) glTexSubImage2D(target, 0, x, y, w, y_max - y, format, type, data); +++ equals to just: +++ glAdjustAlignment(stride); glPixelStorei(GL_UNPACK_ROW_LENGTH, stride / glFmt2bpp(format, type)); glTexSubImage2D(target, 0, x, y, w, h, format, type, data); +++ Any clue ? Thanks for enlightning me .. Cheers, Sven
On Tue, Apr 29, 2008 at 01:11:35PM -0600, Sven Gothel wrote:
To do the unification, one more question ..
Why is that orig Upload function so complicated ? I might miss something here completly, but IMHO:
+++ glAdjustAlignment(stride); glPixelStorei(GL_UNPACK_ROW_LENGTH, stride / glFmt2bpp(format, type)); for (; y + slice <= y_max; y += slice) { glTexSubImage2D(target, 0, x, y, w, slice, format, type, data); data += stride * slice; } if (y < y_max) glTexSubImage2D(target, 0, x, y, w, y_max - y, format, type, data); +++
equals to just:
+++ glAdjustAlignment(stride); glPixelStorei(GL_UNPACK_ROW_LENGTH, stride / glFmt2bpp(format, type)); glTexSubImage2D(target, 0, x, y, w, h, format, type, data); +++
Functionality-wise, yes they are the same. Speed-wise not (necessarily). In tests long ago on a TNT 2 code above could be almost 5% faster.
On Tuesday 29 April 2008 13:31:50 Reimar Döffinger wrote:
On Tue, Apr 29, 2008 at 01:11:35PM -0600, Sven Gothel wrote:
To do the unification, one more question ..
Why is that orig Upload function so complicated ? I might miss something here completly, but IMHO:
+++ glAdjustAlignment(stride); glPixelStorei(GL_UNPACK_ROW_LENGTH, stride / glFmt2bpp(format, type)); for (; y + slice <= y_max; y += slice) { glTexSubImage2D(target, 0, x, y, w, slice, format, type, data); data += stride * slice; } if (y < y_max) glTexSubImage2D(target, 0, x, y, w, y_max - y, format, type, data); +++
equals to just:
+++ glAdjustAlignment(stride); glPixelStorei(GL_UNPACK_ROW_LENGTH, stride / glFmt2bpp(format, type)); glTexSubImage2D(target, 0, x, y, w, h, format, type, data); +++
Functionality-wise, yes they are the same. Speed-wise not (necessarily). In tests long ago on a TNT 2 code above could be almost 5% faster.
Good. I gues that's history now and related to the texture xfer technique. So, one may use pbodma now .. A revised review is coming up, indeed, it looks much simpler now. Thank you. Cheers, Sven
_______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
-- health & wealth mailto:sgothel@jausoft.com ; www : http://www.jausoft.ca ; pgp: http://www.jausoft.com/gpg/ land : +1 (780) 637 3842 ; cell: +1 (780) 952 4481 Timezone MST: EST-2, UTC-7, CET-8 ; MDT: EDT-2, UTC-6, CEDT-8
version 4 .. reflecting Reimar's suggestions. Cheers, Sven -- health & wealth mailto:sgothel@jausoft.com ; www : http://www.jausoft.ca ; pgp: http://www.jausoft.com/gpg/ land : +1 (780) 637 3842 ; cell: +1 (780) 952 4481 Timezone MST: EST-2, UTC-7, CET-8 ; MDT: EDT-2, UTC-6, CEDT-8
On Tuesday 29 April 2008 13:52:02 Sven Gothel wrote:
But the _pic variants will handle stride-conversion for you.
I see .. great, I will double check this - thanks.
So, just drop 'version 4', which I just have send out ..
I will check this now, ...
Well, it will 'just' boil down to use 'memcpy'. The 'old' style of manual optimization is obsolete, I have written about this in the ML as well. The new gcc (>=4.1 ?) will do this for us .. So, it stays with version 4. Please review. Cheers, Sven -- health & wealth mailto:sgothel@jausoft.com ; www : http://www.jausoft.ca ; pgp: http://www.jausoft.com/gpg/ land : +1 (780) 637 3842 ; cell: +1 (780) 952 4481 Timezone MST: EST-2, UTC-7, CET-8 ; MDT: EDT-2, UTC-6, CEDT-8
On Tue, Apr 29, 2008 at 01:58:23PM -0600, Sven Gothel wrote:
On Tuesday 29 April 2008 13:52:02 Sven Gothel wrote:
But the _pic variants will handle stride-conversion for you.
I see .. great, I will double check this - thanks.
So, just drop 'version 4', which I just have send out ..
I will check this now, ...
Well, it will 'just' boil down to use 'memcpy'. The 'old' style of manual optimization is obsolete, I have written about this in the ML as well.
Huh? Written what?
The new gcc (>=4.1 ?) will do this for us ..
Currently, even gcc 2.95 is among the supported compilers, and ignoring gcc 3.4 certainly is not acceptable. Also relying on a compiler to do critical optimizations in my experience has always proven very foolish, compilers are simply not reliable when it comes to optimization. But all this is besides the point, I did not mention the memcpy_pic function for performance reasons but to simplify code, this is a memcpy function that already does all the stride handling, why duplicating the code again? And please do not CC me when answering, I am subscribed to the list after all. Greetings, Reimar Döffinger
On Wednesday 30 April 2008 12:36:40 Reimar Döffinger wrote:
On Tue, Apr 29, 2008 at 01:58:23PM -0600, Sven Gothel wrote:
On Tuesday 29 April 2008 13:52:02 Sven Gothel wrote:
But the _pic variants will handle stride-conversion for you.
I see .. great, I will double check this - thanks.
So, just drop 'version 4', which I just have send out ..
I will check this now, ...
Well, it will 'just' boil down to use 'memcpy'. The 'old' style of manual optimization is obsolete, I have written about this in the ML as well.
Huh? Written what?
The new gcc (>=4.1 ?) will do this for us ..
Currently, even gcc 2.95 is among the supported compilers, and ignoring gcc 3.4 certainly is not acceptable. Also relying on a compiler to do critical optimizations in my experience has always proven very foolish, compilers are simply not reliable when it comes to optimization. But all this is besides the point, I did not mention the memcpy_pic function for performance reasons but to simplify code, this is a memcpy function that already does all the stride handling, why duplicating the code again?
Ok, ack. In one configurable case, we have a single memcpy, where the GPU shall handle the stride, which shall be the fasted way possible. The single memcpy call is appropriate here. In the other configurable case, multiple memcpy, well, ok .. we may can use the memcpy_pic and drop this little self written loop.
And please do not CC me when answering, I am subscribed to the list after all.
Aye Aye :) Cheers, Sven
Greetings, Reimar Döffinger _______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
-- health & wealth mailto:sgothel@jausoft.com ; www : http://www.jausoft.ca ; pgp: http://www.jausoft.com/gpg/ land : +1 (780) 637 3842 ; cell: +1 (780) 952 4481 Timezone MST: EST-2, UTC-7, CET-8 ; MDT: EDT-2, UTC-6, CEDT-8
On Tue, Apr 29, 2008 at 01:44:32PM -0600, Sven Gothel wrote:
version 4 .. reflecting Reimar's suggestions.
I still have some problems understanding the need for most of those changes. While it is really ugly for several reasons and I will have to clean it up once I have a bit more time, could you please use attached patch and tell me what it misses in functionality compared to yours? One "issue" I know is that you may have to use -noslices to make it have an effect. Another thing is probably the GL_UNPACK_ROW_LENGTH support you said ATI drivers are lacking, if this still causes a problems with this patch I'd need a bit more information on this. The usage would be something like mplayer -vo gl:yuv=2:force-pbo -noslices file.avi Greetings, Reimar Döffinger
On Tue, Apr 29, 2008 at 01:44:32PM -0600, Sven Gothel wrote:
version 4 .. reflecting Reimar's suggestions.
I still have some problems understanding the need for most of those changes. While it is really ugly for several reasons and I will have to clean it up once I have a bit more time, Well, we have a different idea what is more complicated and what is not,
On Wednesday 30 April 2008 12:29:13 Reimar Döffinger wrote: that's for sure :) But maybe exactly this will help us to do better. My patch just adds a little PBO helper tool, to ease the PBO usage, utilizing a bit OO style, so we don't have to bother with all the floating around variables. In the end, it all boils down to a little extension of the UpdateTex function, no more no less. The new feature adds a ways to enable DMA xfers. The first DMA is video -> PBO buffer, utilizing memcpy. The 2nd is the GPU's DMA xfer using the PBO for glTexSubImage2D. Why this is usefull for some GPU drivers is very well explained in my initial post. E.g. Nvidia does such a DMA xfer implicit, where AMD doesn't. So this code makes sure DMA is being used. The impact is a performance benefit especially for HD content, from around 80% -> 20%, factor 4, on AMD drivers.
could you please use attached patch and tell me what it misses in functionality compared to yours?
I do. As far I understand your patch, you perform a memcpy, ie DMA xfer, to shape the mp_image_t, so you get rid of the GL_UNPACK_ROW_LENGTH trouble. But this should be redundant, since this PBO case hits if you are already in charge of the texture and video buffer, ie. get_image. Your patch would still use slow PIO in glTexSubImage, because of the lack of direct PBO in get_image. (see below)
One "issue" I know is that you may have to use -noslices to make it have an effect.
Nope, I never ran into the case, where get_image was capable to use the direct PBO fast path. That's why my patch just uses 2 guaranteed DMA xfers, instead of one probable slow PIO texture update. You are right, it would be best, if get_image would allow PBO's .., but all the exceptions checked there hit .. MP_IMGFLAG_READABLE and MP_IMGTYPE_IP were set.
Another thing is probably the GL_UNPACK_ROW_LENGTH support you said ATI drivers are lacking, if this still causes a problems with this patch I'd need a bit more information on this.
Well, it seems it is simply a bug in the current ATI driver, which enforces us to use multiple memcpy, ie. shape the PBO memory so no NPOT stride happen - even though the driver claims to support NPOT.
The usage would be something like mplayer -vo gl:yuv=2:force-pbo -noslices file.avi
Greetings, Reimar Döffinger
-- health & wealth mailto:sgothel@jausoft.com ; www : http://www.jausoft.ca ; pgp: http://www.jausoft.com/gpg/ land : +1 (780) 637 3842 ; cell: +1 (780) 952 4481 Timezone MST: EST-2, UTC-7, CET-8 ; MDT: EDT-2, UTC-6, CEDT-8
On Wed, Apr 30, 2008 at 02:05:57PM -0600, Sven Gothel wrote:
On Tue, Apr 29, 2008 at 01:44:32PM -0600, Sven Gothel wrote:
version 4 .. reflecting Reimar's suggestions.
I still have some problems understanding the need for most of those changes. While it is really ugly for several reasons and I will have to clean it up once I have a bit more time, Well, we have a different idea what is more complicated and what is not,
On Wednesday 30 April 2008 12:29:13 Reimar Döffinger wrote: that's for sure :)
Well, the criteria I used is simple: my patch adds about 10 non-trivial lines of code, yours 50.
My patch just adds a little PBO helper tool, to ease the PBO usage, utilizing a bit OO style, so we don't have to bother with all the floating around variables.
Reducing the number of global variables is a good goal, but doing this in a patch that fixes a performance issue makes it really hard to understand where the performance issue is, what causes it, and what is the simplest way to fix it.
The new feature adds a ways to enable DMA xfers. The first DMA is video -> PBO buffer, utilizing memcpy. The 2nd is the GPU's DMA xfer using the PBO for glTexSubImage2D.
How do you define DMA? I learned DMA as data transfer between (usually) main memory and a peripheral device under the control of the device, thus leaving the CPU free. To me, the memcpy used in this case seems to fail all these criteria...
The impact is a performance benefit especially for HD content, from around 80% -> 20%, factor 4, on AMD drivers.
This only restates what the whole patch achieves, but it does not clarify _which parts_ of the patch are relevant for this. E.g. the current PBO handling code uses 1 PBO for YV12 mode, your patch changes this to use 3. Does this have any effect on speed? And what about the changes to draw_slice? Do they improve speed? Was that code tested at all? May it actually be slower that the original code? This case is less critical since the new code can be disabled, but having all these changes to code paths that are used in very different situations in one single patch is likely to find out where issues are if any arise.
could you please use attached patch and tell me what it misses in functionality compared to yours?
I do.
As far I understand your patch, you perform a memcpy, ie DMA xfer, to shape the mp_image_t, so you get rid of the GL_UNPACK_ROW_LENGTH trouble. But this should be redundant, since this PBO case hits if you are already in charge of the texture and video buffer, ie. get_image.
Your patch would still use slow PIO in glTexSubImage, because of the lack of direct PBO in get_image. (see below)
I'd prefer if you test, because it should be exactly the other way round, it should be faster (and with my nVidia card vo CPU usage does drop from 14% to 6%) but it should not fix the GL_UNPACK_ROW_LENGTH issue (though it probably is trivial to fix).
Another thing is probably the GL_UNPACK_ROW_LENGTH support you said ATI drivers are lacking, if this still causes a problems with this patch I'd need a bit more information on this.
Well, it seems it is simply a bug in the current ATI driver, which enforces us to use multiple memcpy, ie. shape the PBO memory so no NPOT stride happen - even though the driver claims to support NPOT.
What is NPOT supposed to mean? My only interpretation of NPOT would be non-power-of-two but that makes no sense. Do you mean it requires 4-byte alignment or something like that? In that case, the problem is more likely to be in the GL_UNPACK_ALIGNMENT handling... Greetings, Reimar Döffinger
On Wednesday 30 April 2008 14:44:40 Reimar Döffinger wrote:
On Wed, Apr 30, 2008 at 02:05:57PM -0600, Sven Gothel wrote:
On Tue, Apr 29, 2008 at 01:44:32PM -0600, Sven Gothel wrote:
version 4 .. reflecting Reimar's suggestions.
I still have some problems understanding the need for most of those changes. While it is really ugly for several reasons and I will have to clean it up once I have a bit more time, Well, we have a different idea what is more complicated and what is not,
On Wednesday 30 April 2008 12:29:13 Reimar Döffinger wrote: that's for sure :)
Well, the criteria I used is simple: my patch adds about 10 non-trivial lines of code, yours 50.
My patch just adds a little PBO helper tool, to ease the PBO usage, utilizing a bit OO style, so we don't have to bother with all the floating around variables.
Reducing the number of global variables is a good goal, but doing this in a patch that fixes a performance issue makes it really hard to understand where the performance issue is, what causes it, and what is the simplest way to fix it.
It depends. Proper management for multiple PBO's indicates a little toolkit. But .. maybe I am completly wrong, possible, and the whole thing I did is not necessary - we will see.
The new feature adds a ways to enable DMA xfers. The first DMA is video -> PBO buffer, utilizing memcpy. The 2nd is the GPU's DMA xfer using the PBO for glTexSubImage2D.
How do you define DMA? I learned DMA as data transfer between (usually) main memory and a peripheral device under the control of the device, thus leaving the CPU free. To me, the memcpy used in this case seems to fail all these criteria...
DMA can be any memory transfer not handled by the CPU in PIO mode. On a system, there are many DMA controller available. The GPU one usually is able to handle: GPUMem <-> SystemMem GPUMem <-> GPUMem maybe even (AMD GPU's can do this for sure): SystemMem <-> SystemMem The system memory controller under some architectures is able to do the same, at least SystemMem <-> SystemMem. This maybe utilized by memcpy, to my knowledge it is on ia/x86 platforms. Even though memcpy would be performed in PIO mode, using PBO's for glTexSubImage ensures at least this one is using DMA.
The impact is a performance benefit especially for HD content, from around 80% -> 20%, factor 4, on AMD drivers.
This only restates what the whole patch achieves, but it does not clarify _which parts_ of the patch are relevant for this.
Right, let's drop the details of the patch and let's discuss the issue itself.
E.g. the current PBO handling code uses 1 PBO for YV12 mode, your patch changes this to use 3. Does this have any effect on speed? No. It just ensures, that we setup the PBO only once. Of course, instead of the 3 (full-size + 2 half-size), you may create and use the fullsize only, since they are used sequentially. That would be fine .. IMHO.
And what about the changes to draw_slice? Do they improve speed? Yes. It ensures at least one guaranteed DMA transfer (see above).
Was that code tested at all? May it actually be slower that the original code?
Actually this path is not that critical, at least not with my test cases. Ie. I do not have HD mpeg content to decode with mpeg12, ffmpeg12 doesn't use slicing, I have observed. Regarding the slicing part, I see that you always copy the whole video frame. So we may can drop the previous slices/copies and do it only on the whole image, ie when y+slice==h ?
This case is less critical since the new code can be disabled, Right.
but having all these changes to code paths that are used in very different situations in one single patch is likely to find out where issues are if any arise.
I totally agree that we have to test and discuss all of this, that's why we love code reviews. And having more test data/information, ie HD codecs, etc, I like to run a few test with those as well. However, I believe my patch doesn't change anything fundamental/functional, and it is optional anyway -> no impact with the default pbodma=0. The latter stament should be verified first (IMHO), because it is important that I don't break anything, of course. And let's drive our performance discussion forward .. it's fun to do so.
could you please use attached patch and tell me what it misses in functionality compared to yours?
I do.
As far I understand your patch, you perform a memcpy, ie DMA xfer, to shape the mp_image_t, so you get rid of the GL_UNPACK_ROW_LENGTH trouble. But this should be redundant, since this PBO case hits if you are already in charge of the texture and video buffer, ie. get_image.
Your patch would still use slow PIO in glTexSubImage, because of the lack of direct PBO in get_image. (see below)
I'd prefer if you test, because it should be exactly the other way round,
Well, at least I have catched that 'GL_UNPACK_ROW_LENGTH fix' :)
it should be faster (and with my nVidia card vo CPU usage does drop from 14% to 6%) but it should not fix the GL_UNPACK_ROW_LENGTH issue (though it probably is trivial to fix).
I see. But NVidia's driver is already using DMA xfer's for a simple texture upload, even without PBO's. The GL_UNPACK_ROW_LENGTH ATI bug currently only appears for NPOT strides using PBO's. Normal texture uploads just works.
Another thing is probably the GL_UNPACK_ROW_LENGTH support you said ATI drivers are lacking, if this still causes a problems with this patch I'd need a bit more information on this.
Well, it seems it is simply a bug in the current ATI driver, which enforces us to use multiple memcpy, ie. shape the PBO memory so no NPOT stride happen - even though the driver claims to support NPOT.
What is NPOT supposed to mean? My only interpretation of NPOT would be non-power-of-two
right. Actually they use it for non power of two textures, but we get the drift, right.
but that makes no sense. Do you mean it requires 4-byte alignment or something like that? Nope not a n-byte alignment, but a required dimension, where each side is = pow(2,n). You know, the so very old texture requirement.
In that case, the problem is more likely to be in the GL_UNPACK_ALIGNMENT handling...
that's what I tried to communicate - but only related to PBO xfers. This problem is only an issue using PBO's for tex uploads and is 'just' a bug for ATI cards, hence the pbodma option for multiple memcpy. The root problem is the slow texture upload on, let's say, non NVidia cards/driver, which just don't use DMA implicit. Cheers, Sven
Greetings, Reimar Döffinger
On Wed, Apr 30, 2008 at 03:48:40PM -0600, Sven Gothel wrote:
On Wednesday 30 April 2008 14:44:40 Reimar Döffinger wrote:
On Wed, Apr 30, 2008 at 02:05:57PM -0600, Sven Gothel wrote:
My patch just adds a little PBO helper tool, to ease the PBO usage, utilizing a bit OO style, so we don't have to bother with all the floating around variables.
Reducing the number of global variables is a good goal, but doing this in a patch that fixes a performance issue makes it really hard to understand where the performance issue is, what causes it, and what is the simplest way to fix it.
It depends. Proper management for multiple PBO's indicates a little toolkit.
Though your suggestion makes PBOs dependent on textures. It is already problematic for get_image (when the U and V planes may be required to follow directly after the Y plane) and it does work badly or possibly not at all for e.g. vertexes, and the same when one texture needs multiple PBOs (as for MPEG codecs where the additional PBOs would be needed to store the reference frames for direct rendering - that is assuming OpenGL can provide PBO buffers that can be read fast enough). Which is why I would tend more towards a malloc-like interface to PBOs if I write a framework.
How do you define DMA? I learned DMA as data transfer between (usually) main memory and a peripheral device under the control of the device, thus leaving the CPU free. To me, the memcpy used in this case seems to fail all these criteria...
DMA can be any memory transfer not handled by the CPU in PIO mode. On a system, there are many DMA controller available.
The GPU one usually is able to handle: GPUMem <-> SystemMem
Ok.
GPUMem <-> GPUMem
Where did you get this information from? I don't see much sense and quite a few technical problems for a GPU memory to GPU memory DMA engine (unless you count the GPU itself as a DMA engine).
maybe even (AMD GPU's can do this for sure): SystemMem <-> SystemMem
Huh? Why would they implement that, doing memory to memory copies over the PCIe bus? That would be really slow...
The system memory controller under some architectures is able to do the same, at least SystemMem <-> SystemMem.
I'd really like to see some proof of that. Unless you consider what the Cell does as system memory to system memory DMA, though usually it is described as system memory to local cache transfer (and the other way round).
This maybe utilized by memcpy, to my knowledge it is on ia/x86 platforms.
No, certainly not. You can check the glibc sources. Or just disassemble your system's libc.
Even though memcpy would be performed in PIO mode, using PBO's for glTexSubImage ensures at least this one is using DMA.
For all I know, there actually is not guarantee that PBO transfer operations will use DMA. They usually will, but e.g. if you create so many PBOs that they no longer fit in the DMA area they will use normal copy. Though it might be that the problem of fixed-size DMA areas has been finally solved, I haven't paid attention to that, and there certainly are enough possible solutions.
E.g. the current PBO handling code uses 1 PBO for YV12 mode, your patch changes this to use 3. Does this have any effect on speed? No. It just ensures, that we setup the PBO only once. Of course, instead of the 3 (full-size + 2 half-size), you may create and use the fullsize only, since they are used sequentially. That would be fine .. IMHO.
I meant one 3/2 size PBO, as it is used by get_image. Either way I too doubt it makes a difference on speed.
Regarding the slicing part, I see that you always copy the whole video frame. So we may can drop the previous slices/copies and do it only on the whole image, ie when y+slice==h ?
Only if it is carefully benchmarked and shown not to be slower on a wide range of systems.
Well, it seems it is simply a bug in the current ATI driver, which enforces us to use multiple memcpy, ie. shape the PBO memory so no NPOT stride happen - even though the driver claims to support NPOT.
What is NPOT supposed to mean? My only interpretation of NPOT would be non-power-of-two right. Actually they use it for non power of two textures, but we get the drift, right.
So you mean this is only a problem with rectangle=2 ? Haven't seen that make a useful difference anyway... If that's all I'd prefer to leave it broken. Greetings, Reimar Döffinger
I'm pretty late about this, but you're probably still interrested. On May 03, 08 15:10:17 +0200, Reimar Döffinger wrote:
DMA can be any memory transfer not handled by the CPU in PIO mode. On a system, there are many DMA controller available.
The GPU one usually is able to handle: GPUMem <-> SystemMem GPUMem <-> GPUMem
Where did you get this information from? I don't see much sense and
ScreenToScreenCopy, EXT_pimap_to_texture, glCopyTexImage2D etc. use the GPU's DMA engine for that. GPU's can even DMA from memory to register space in some occasions...
quite a few technical problems for a GPU memory to GPU memory DMA engine (unless you count the GPU itself as a DMA engine).
The GPU *has* a DMA engine, it's only a small part of it.
maybe even (AMD GPU's can do this for sure): SystemMem <-> SystemMem Huh? Why would they implement that, doing memory to memory copies over the PCIe bus? That would be really slow...
1st) Consistency (you don't have to check pointers first) 2nd) Textures might be tiled (e.g. in AGP memory) and you might want to access them in an untiled manner
The system memory controller under some architectures is able to do the same, at least SystemMem <-> SystemMem.
Typically not for PCs.
For all I know, there actually is not guarantee that PBO transfer operations will use DMA. They usually will, but e.g. if you create so many PBOs that they no longer fit in the DMA area they will use normal copy. Though it might be that the problem of fixed-size DMA areas has been finally solved, I haven't paid attention to that, and there certainly are enough possible solutions.
The main source of confusion is that nowadays memcpy *can* be faster than DMA in some cases, if setting up the DMA engine (and potentially converting data) is slow... Typically, for texture uploads at least one memcpy is always involved. But typically in WriteBack or WriteThrough/WriteCombined memory, which might be faster than the mapped frame buffer. Might. Better to not argue about DMA or not, but used total and processor time. CU Matthias -- Matthias Hopf <mhopf@suse.de> __ __ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ mat@mshopf.de Phone +49-911-74053-715 __) |_| __) |__ R & D www.mshopf.de
Hi Sven Gothel! On 2008.04.29 at 13:44:32 -0600, Sven Gothel wrote next:
version 4 .. reflecting Reimar's suggestions.
What driver are you using? What kind of benchmark showed performance increase? Because with ati X1900 and fglrx driver rendering is about 50% slower both with pbodma=1 and pbodma=2 options (compared to pbodma=0 or unpatched mplayer). That's according to mplayer stats, which sometimes doesn't show the right things, but I guess you can believe them in quick and dirty test. I.e. when playing 30-40mbit/s 1080p mpeg2 scene on core 2 duo @ 3.15 ghz with -vf-clr -vo gl:yuv=2, a few seconds after seeking: A: 255.7 V: 255.7 A-V: 0.000 ct: 0.191 657/657 22% 7% 1.2% 0 0 49% (cpu usage in vo is at 7-8%) same scene with -vo gl:yuv=2:pbodma=1: A: 249.3 V: 249.3 A-V: 0.000 ct: 0.178 177/177 21% 13% 1.0% 0 0 47% (cpu usage in vo is at 12-13%) For the record, with xv (texturedvideo implementation) cpu usage is 5-6%. Though I prefer gl because of colored subtitles and high-quality font rendering in ssa/ass. PS Could it be that your problem (with dma not being used) is specific to R6xx hardware? Because gl output driver performs perfectly on R3xx and R5xx class hardware and doesn't use much more cpu than xv. -- Vladimir
Hello, On Sat, May 03, 2008 at 12:13:01AM +0400, Vladimir Mosgalin wrote:
Because with ati X1900 and fglrx driver rendering is about 50% slower both with pbodma=1 and pbodma=2 options (compared to pbodma=0 or unpatched mplayer). That's according to mplayer stats, which sometimes doesn't show the right things, but I guess you can believe them in quick and dirty test.
Could you check with my patch as well? Btw. what kind of CPU are you using? Mine is a dual core, I think this might make a difference... Greetings, Reimar Döffinger
Hi Reimar Döffinger! On 2008.05.02 at 22:30:47 +0200, Reimar Döffinger wrote next:
Because with ati X1900 and fglrx driver rendering is about 50% slower both with pbodma=1 and pbodma=2 options (compared to pbodma=0 or unpatched mplayer). That's according to mplayer stats, which sometimes doesn't show the right things, but I guess you can believe them in quick and dirty test.
Could you check with my patch as well? Btw. what kind of CPU are you
You know what? It works! Doesn't require -noslices, too (it makes no difference). CPU usage is at 5% with your patch (4-5%), which is among the lowest I can get with any video driver. I never thought about testing it because I thought your patch is like a simpler version of the original one, and surely simplified version wouldn't work where original has failed. Seems like I was wrong.
using? Mine is a dual core, I think this might make a difference...
Yep, mine is dual core too. E6600. Anyway since mplayer can't use more than one core and fglrx driver doesn't seem to use multiple cores too (e.g. glxgears either uses up to 100% of a single core while X uses 20% of the other or they can use single core in 80/20 proportion - with stupid linux scheduler switching between two modes), I'm not sure it makes too much difference.. -- Vladimir
Hi Reimar Döffinger! On 2008.05.02 at 22:30:47 +0200, Reimar Döffinger wrote next: I've found a strange video which produces very distorted picture with -vo gl:yuv=2:force-pbo. -vo gl, gl:yuv=<anything> and gl:force-pbo work fine, but combination of yuv and force-pbo options produces strange result. Adding -noslices changes distortion a bit, but doesn't solve it. I've uploaded it to upload.mplayerhq.hu/incoming as gl_force-pbo_problems.avi. Alternatively, you can download it from http://slil.ru/25760498 Screenshot of what I see is here: http://img217.imageshack.us/img217/8466/screenshotmplayerob2.png Can you please look at it? I must say that I've watched quite a bit of HD and SD videos with yuv=2:force-pbo suboptions already and it works great. I've never imagined to stumble upon this sample. -- Vladimir
Hello, On Tue, May 06, 2008 at 04:44:30AM +0400, Vladimir Mosgalin wrote:
I've found a strange video which produces very distorted picture with -vo gl:yuv=2:force-pbo. -vo gl, gl:yuv=<anything> and gl:force-pbo work fine, but combination of yuv and force-pbo options produces strange result. Adding -noslices changes distortion a bit, but doesn't solve it.
Oh, sorry, you are using a buggy version anyway. Try attached patch, I don't know if it will help though. Also note that in latest SVN -vo gl supports -dr for B-frames (in MPEG2/4), which might help quite a bit if you have videos in that format. Greetings, Reimar Döffinger
Hi Reimar Döffinger! On 2008.05.06 at 08:53:21 +0200, Reimar Döffinger wrote next:
Oh, sorry, you are using a buggy version anyway. Try attached patch, I don't know if it will help though.
No, it doesn't. I applied it to the older version, though, not fresh svn. (well svn has no changes in vo_gl).
Also note that in latest SVN -vo gl supports -dr for B-frames (in MPEG2/4), which might help quite a bit if you have videos in that format.
Hmm.. Dunno why anybody would want -dr, I doubt it does anything useful anyway. I used it when I had P1-166, but always turned it off since that - the effect wasn't noticeable, the only thing I remember about -dr is that it could make osd flicker under some conditions. Also, wasn't it making gl output much slower (and producing some big fat warning when used with gl vo? Don't observe it now though..)?
+ if (force_pbo && !(mpi->flags & MP_IMGFLAG_DIRECT) && !gl_bufferptr[0] && get_image(&mpi2) == VO_TRUE) {
I assume you meant gl_bufferptr, since you can't dereference a void* ? -- Vladimir
On Tue, May 06, 2008 at 11:11:24PM +0400, Vladimir Mosgalin wrote:
On 2008.05.06 at 08:53:21 +0200, Reimar Döffinger wrote next:
Oh, sorry, you are using a buggy version anyway. Try attached patch, I don't know if it will help though.
No, it doesn't. I applied it to the older version, though, not fresh svn. (well svn has no changes in vo_gl).
That's bad because I can not reproduce the problem so far.
Also note that in latest SVN -vo gl supports -dr for B-frames (in MPEG2/4), which might help quite a bit if you have videos in that format.
Hmm.. Dunno why anybody would want -dr, I doubt it does anything useful anyway. I used it when I had P1-166, but always turned it off since that - the effect wasn't noticeable, the only thing I remember about -dr is that it could make osd flicker under some conditions.
The flicker is due to xv not handling it right. For gl, -dr basically does exactly what force-pbo does, just without the additional memcpy - which in turn means it does not work for H.264 and reference frames for the other formats.
Also, wasn't it making gl output much slower (and producing some big fat warning when used with gl vo? Don't observe it now though..)?
It produced that warning when the card did not support PBO, and in that case it did fall back to the same behaviour as without -dr, so -dr certainly did never slow anything down (well, except for during some buggy revisions of vo_gl).
+ if (force_pbo && !(mpi->flags & MP_IMGFLAG_DIRECT) && !gl_bufferptr[0] && get_image(&mpi2) == VO_TRUE) {
I assume you meant gl_bufferptr, since you can't dereference a void* ?
Yes, that was left over from some experiments. Unfortunately at least nVidia still can not provide PBOs that can be read efficiently, otherwise we might have been able to avoid all this messy force-pbo and go with a proper and faster -dr solution...
Hi Reimar Döffinger! On 2008.05.06 at 21:53:59 +0200, Reimar Döffinger wrote next:
No, it doesn't. I applied it to the older version, though, not fresh svn. (well svn has no changes in vo_gl).
That's bad because I can not reproduce the problem so far.
Hm. Well, maybe you would be able to guess where the problem can lie? Here are some interesting facts. First, video height doesn't seem to produce problem - only width matters. Resolutions not divisible by 32 are affected. When width % 32 == 30 the picture is "skewed" slightly, with width % 32 between 14-18 it's skewed a lot, but looks somewhat clear. At other values distortion grows. Also the exact look of distortion depends on video height. Both chroma and luma are distorted, also they don't match each other when any kind of distortion occurs. Funny, but osd and vobsubs, even colored aren't affected at all! I created a bunch of screenshots to show these effects by running mplayer -vf test,scale=x:512, where x is from 638 to 672 (I paused mplayer by hand, so source frames could be a bit different). Only even values, since mplayer automatically increases width by 1 when odd value is supplied. Also I made 670:1000 shot for comparison with 670:512 and shot showing chroma/luma mismatch and subs. You can download them at http://slil.ru/25764374 (1.6 mb). -- Vladimir
On Wed, May 07, 2008 at 02:04:14AM +0400, Vladimir Mosgalin wrote:
First, video height doesn't seem to produce problem - only width matters. Resolutions not divisible by 32 are affected. When width % 32 == 30 the picture is "skewed" slightly, with width % 32 between 14-18 it's skewed a lot, but looks somewhat clear. At other values distortion grows. Also the exact look of distortion depends on video height.
Both chroma and luma are distorted, also they don't match each other when any kind of distortion occurs. Funny, but osd and vobsubs, even colored aren't affected at all!
That is not surprising. What is very surprising to me is that resolutions would need to be divisible by 32 for things to work, that is an unusually large number. You could try changing
mpi2.width = mpi2.w; to mpi2.width = (mpi2.w + 31) & ~31; but that wastes quite a lot of space. I forgot, which card/drivers are you using? I am using "nVidia Corporation G70 [GeForce 7600 GS]" with driver version 169.12
Greetings, Reimar Döffinger
Hi Reimar Döffinger! On 2008.05.07 at 08:53:46 +0200, Reimar Döffinger wrote next:
That is not surprising. What is very surprising to me is that resolutions would need to be divisible by 32 for things to work, that is an unusually large number. You could try changing
mpi2.width = mpi2.w; to mpi2.width = (mpi2.w + 31) & ~31;
Luma is fixed, chroma is a mess.
but that wastes quite a lot of space.
Space.. where? Frame in memory becomes a bit larger?
I forgot, which card/drivers are you using? I am using "nVidia Corporation G70 [GeForce 7600 GS]" with driver version 169.12
Radeon X1900, fglrx 8.443.1 (latest) under fedora 8 x86-64. -- Vladimir
On Wed, May 07, 2008 at 07:10:21PM +0400, Vladimir Mosgalin wrote:
On 2008.05.07 at 08:53:46 +0200, Reimar Döffinger wrote next:
That is not surprising. What is very surprising to me is that resolutions would need to be divisible by 32 for things to work, that is an unusually large number. You could try changing
mpi2.width = mpi2.w; to mpi2.width = (mpi2.w + 31) & ~31;
Luma is fixed, chroma is a mess.
I guessed so. Use 63 instead of 31.
but that wastes quite a lot of space.
Space.. where? Frame in memory becomes a bit larger?
Wherever the driver puts the PBO up to 93 bytes per video line are wasted.
I forgot, which card/drivers are you using? I am using "nVidia Corporation G70 [GeForce 7600 GS]" with driver version 169.12
Radeon X1900, fglrx 8.443.1 (latest) under fedora 8 x86-64.
Ok, then it's probably the issue mentioned in the original patch submission I'd guess... Greetings, Reimar Döffinger
Hi Reimar Döffinger! On 2008.05.07 at 17:40:31 +0200, Reimar Döffinger wrote next:
Luma is fixed, chroma is a mess.
I guessed so. Use 63 instead of 31.
Yes now it works ;) Thanks. Is that a proper fix?
but that wastes quite a lot of space.
Space.. where? Frame in memory becomes a bit larger?
Wherever the driver puts the PBO up to 93 bytes per video line are wasted.
Hmm.. Well that's like, 5-10% of full frame size? It probably shouldn't matter..
I forgot, which card/drivers are you using? I am using "nVidia Corporation G70 [GeForce 7600 GS]" with driver version 169.12
Radeon X1900, fglrx 8.443.1 (latest) under fedora 8 x86-64.
Ok, then it's probably the issue mentioned in the original patch submission I'd guess...
But the original patch completely failed for me, and I never observed the problems described there. -- Vladimir
On Wed, May 07, 2008 at 07:57:31PM +0400, Vladimir Mosgalin wrote:
On 2008.05.07 at 17:40:31 +0200, Reimar Döffinger wrote next:
Luma is fixed, chroma is a mess.
I guessed so. Use 63 instead of 31.
Yes now it works ;) Thanks. Is that a proper fix?
No, the proper fix is a bug report to ATI I think. But it probably is an acceptable hack.
but that wastes quite a lot of space.
Space.. where? Frame in memory becomes a bit larger?
Wherever the driver puts the PBO up to 93 bytes per video line are wasted.
Hmm.. Well that's like, 5-10% of full frame size? It probably shouldn't matter..
about 3% at HD, 9% at DVD. Not that much, but a lot for something that should not be necessary. Greetings, Reimar Döffinger
Hello, On Wed, May 07, 2008 at 07:57:31PM +0400, Vladimir Mosgalin wrote:
On 2008.05.07 at 17:40:31 +0200, Reimar Döffinger wrote next:
Luma is fixed, chroma is a mess.
I guessed so. Use 63 instead of 31.
Yes now it works ;) Thanks. Is that a proper fix?
Since either me or ATI are doing something quite wrong, I do not like to just hide this issue. So I propose attached patch, where you will probably need to use the ati-hack suboption in addition to force-pbo. Greetings, Reimar Döffinger
On Tue, May 06, 2008 at 08:53:21AM +0200, Reimar Döffinger wrote:
On Tue, May 06, 2008 at 04:44:30AM +0400, Vladimir Mosgalin wrote:
I've found a strange video which produces very distorted picture with -vo gl:yuv=2:force-pbo. -vo gl, gl:yuv=<anything> and gl:force-pbo work fine, but combination of yuv and force-pbo options produces strange result. Adding -noslices changes distortion a bit, but doesn't solve it.
Oh, sorry, you are using a buggy version anyway. Try attached patch, I don't know if it will help though. Also note that in latest SVN -vo gl supports -dr for B-frames (in MPEG2/4), which might help quite a bit if you have videos in that format.
Sorry, that patch was broken. Attached patch works. I could not reproduce your problems with this version, and the first version crashed for me for quite a lot of files, but I had no corruption. So no idea if it fixes everything... Greetings, Reimar Döffinger
Hi Reimar Döffinger! On 2008.05.02 at 22:30:47 +0200, Reimar Döffinger wrote next: Hm, I've discovered that you can get this effect on any video by using -vf scale=700:390. Or 700:400. Or 720:480 (heh). On the other hand, HD resolutions and common SD ones play fine. I picked up some random video which was 704x480 and it was fine, too (and obviously, 704:480 doesn't produce this effect). -- Vladimir
participants (4)
-
Matthias Hopf -
Reimar Döffinger -
Sven Gothel -
Vladimir Mosgalin