<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Le 05/04/2018 à 10:05, Rémi Denis-Courmont a écrit :<br>
<blockquote type="cite"
cite="mid:73D31D36-A12E-44DE-9F7B-2CF09688EBB9@remlab.net">
<pre wrap="">Le 5 avril 2018 09:55:25 GMT+03:00, Steve Lhomme <a class="moz-txt-link-rfc2396E" href="mailto:robux4@ycbcr.xyz"><robux4@ycbcr.xyz></a> a écrit :
</pre>
<blockquote type="cite">
<pre wrap="">This is a rebased/reworked version of
<a class="moz-txt-link-freetext" href="https://patches.videolan.org/patch/19128/">https://patches.videolan.org/patch/19128/</a>
Also since this is done in picture_Setup() I wonder if we should also
update the i_width/i_height of the picture accordingly. Right now a
picture that has its height modified for padding has its plane lines
correct but the height reported doesn't match. Leading to potential
misdetection of padding/alignment.
Le 05/04/2018 à 08:51, Steve Lhomme a écrit :
</pre>
<blockquote type="cite">
<pre wrap="">No need for extra lines when there's already padding. Plus it may
</pre>
</blockquote>
<pre wrap="">break the
</pre>
<blockquote type="cite">
<pre wrap="">alignment of the secondary planes.
---
src/misc/picture.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/misc/picture.c b/src/misc/picture.c
index 7506e47725..7ce1170702 100644
--- a/src/misc/picture.c
+++ b/src/misc/picture.c
@@ -133,17 +133,17 @@ int picture_Setup( picture_t *p_picture, const
</pre>
</blockquote>
<pre wrap="">video_format_t *restrict fmt )
</pre>
<blockquote type="cite">
<pre wrap="">
unsigned width, height;
+ /* Hack: append two scan lines for some SIMD assembler */
+ if (unlikely(add_overflow(fmt->i_height, 2 * i_ratio_h,
</pre>
</blockquote>
<pre wrap="">&height)))
</pre>
<blockquote type="cite">
<pre wrap="">+ return VLC_EGENERIC;
+
if (unlikely(add_overflow(fmt->i_width, i_modulo_w - 1,
</pre>
</blockquote>
<pre wrap="">&width))
</pre>
<blockquote type="cite">
<pre wrap="">- || unlikely(add_overflow(fmt->i_height, i_modulo_h - 1,
</pre>
</blockquote>
<pre wrap="">&height)))
</pre>
<blockquote type="cite">
<pre wrap="">+ || unlikely(add_overflow(height, i_modulo_h - 1, &height)))
return VLC_EGENERIC;
width = width / i_modulo_w * i_modulo_w;
height = height / i_modulo_h * i_modulo_h;
- /* Hack: append two scan lines for some SIMD assembler */
- if (unlikely(add_overflow(height, 2 * i_ratio_h, &height)))
- return VLC_EGENERIC;
-
/* plane_t uses 'int'. */
if (unlikely(width > INT_MAX) || unlikely(height > INT_MAX))
return VLC_EGENERIC;
</pre>
</blockquote>
<pre wrap="">
_______________________________________________
vlc-devel mailing list
To unsubscribe or modify your subscription options:
<a class="moz-txt-link-freetext" href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a>
</pre>
</blockquote>
<pre wrap="">
This works if SIMD simply assumes two extra lines after visible lines. But it does not work if SIMD assumes two extra lines after the aligned visible lines. This could be an algorithm operating on macro blocks and expecting some extra padding...
Basically, this patch assumes that either lines count must be over-aligned or there must be extra lines, but not both for any given algorithm. Not that I'd know if there is or ever will be such a case in underlying libraries.</pre>
</blockquote>
<br>
Yes, and that's also a problem. We keep on misaligning every buffer
(we do the alignment and then add 2 lines, breaking the <span
style=" color:#000000;">i_modulo_h</span>) for some hypothetical
case that may not exist anymore. For the ASM code I've looked
at/modified it was always possible to stop processing after the
visible lines. I'm not sure if there's anything that requires that
anymore.<br>
<br>
And since with 4.0 modules might each handle their pool, the ones
that need more specific alignment may deal with it themselves.<br>
<br>
<blockquote type="cite"
cite="mid:73D31D36-A12E-44DE-9F7B-2CF09688EBB9@remlab.net">
<pre wrap="">
Regardless, I don't see how this fixes anything. The alignment of next planes should not be dependent on the number of lines of the previous planes. IMO, all planes should be individually page-aligned.</pre>
</blockquote>
<br>
This is the case for QSV<br>
<a class="moz-txt-link-freetext" href="http://git.videolan.org/?p=vlc.git;a=blob;f=modules/codec/qsv.c;h=0f865dd4ab748fdc4cb2e8685b31547597ed0c55;hb=HEAD#l761">http://git.videolan.org/?p=vlc.git;a=blob;f=modules/codec/qsv.c;h=0f865dd4ab748fdc4cb2e8685b31547597ed0c55;hb=HEAD#l761</a><br>
<br>
Due to internal optimizations they require that the UV planes follow
extactly the Y plane for NV12 textures and that they are aligned to
16 or 32 pixels. There might be cases where this assert doesn't work
if the added 2 lines don't fall in a bigger 16/32 rounding.<br>
<br>
<blockquote type="cite"
cite="mid:73D31D36-A12E-44DE-9F7B-2CF09688EBB9@remlab.net">
<pre wrap="">
</pre>
</blockquote>
<br>
</body>
</html>