[FFmpeg-devel] [PATCH 1/3] lavc/h274: fix PRNG definition

[FFmpeg-devel] [PATCH 1/3] lavc/h274: fix PRNG definition

[Niklas Haas]

From: Niklas Haas <git@haasn.dev>

The spec specifies x^31 + x^3 + 1 as the polynomial, but the diagram in
Figure 1-1 omits the +1 offset. The initial implementation was based on
the diagram, but this is wrong (produces subtly incorrect results).
---
 libavcodec/h274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/h274.c b/libavcodec/h274.c
index a69f9411429..fc111cdb50d 100644
--- a/libavcodec/h274.c
+++ b/libavcodec/h274.c
@@ -38,7 +38,7 @@ static void prng_shift(uint32_t *state)
 {
     // Primitive polynomial x^31 + x^3 + 1 (modulo 2)
     uint32_t x = *state;
-    uint8_t feedback = (x >> 2) ^ (x >> 30);
+    uint8_t feedback = 1u ^ (x >> 2) ^ (x >> 30);
     *state = (x << 1) | (feedback & 1u);
 }
 
-- 
2.42.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/3] lavc/h274: correct grain DB indices

[Niklas Haas]

From: Niklas Haas <git@haasn.dev>

The spec specified indices in the order [x][y], but our code follows the
traditional C convention of [y][x]. This was not correctly account for
when calculating the base index of the grain database access.
---
 libavcodec/h274.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libavcodec/h274.c b/libavcodec/h274.c
index fc111cdb50d..2388826d547 100644
--- a/libavcodec/h274.c
+++ b/libavcodec/h274.c
@@ -259,11 +259,11 @@ int ff_h274_apply_film_grain(AVFrame *out_frame, const AVFrame *in_frame,
         // only advanced in 16x16 blocks, so use a nested loop
         for (int y = 0; y < height; y += 16) {
             for (int x = 0; x < width; x += 16) {
-                uint16_t y_offset = (seed >> 16) % 52;
-                uint16_t x_offset = (seed & 0xFFFF) % 56;
+                uint16_t x_offset = (seed >> 16) % 52;
+                uint16_t y_offset = (seed & 0xFFFF) % 56;
                 const int invert = (seed & 0x1);
-                y_offset &= 0xFFFC;
-                x_offset &= 0xFFF8;
+                x_offset &= 0xFFFC;
+                y_offset &= 0xFFF8;
                 prng_shift(&seed);
 
                 for (int yy = 0; yy < 16 && y+yy < height; yy += 8) {
-- 
2.42.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/3] lavc/h274: fix comment (cosmetic)

[Niklas Haas]

From: Niklas Haas <git@haasn.dev>

Either the average, or the sum right-shifted. Not the average
right-shifted.
---
 libavcodec/h274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/h274.c b/libavcodec/h274.c
index 2388826d547..a5caf09564d 100644
--- a/libavcodec/h274.c
+++ b/libavcodec/h274.c
@@ -110,7 +110,7 @@ static void init_slice(H274FilmGrainDatabase *database, uint8_t h, uint8_t v)
     init_slice_c(database->db[h][v], h, v, database->slice_tmp);
 }
 
-// Computes the average of an 8x8 block, right-shifted by 6
+// Computes the average of an 8x8 block
 static uint16_t avg_8x8_c(const uint8_t *in, int in_stride)
 {
     uint16_t avg[8] = {0}; // summing over an array vectorizes better
-- 
2.42.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] lavc/h274: fix PRNG definition

[Niklas Haas]

Context: I compared our implementation against a H.274 reference stream.
Identified this issues as a result.

This is still not bit-exact, we round incorrectly in some cases (~2% of
output pixels are +1 or -1). I'm not sure where we pick up the remaining
error from..
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] lavc/h274: fix PRNG definition

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1244 bytes --]

On Wed, Sep 27, 2023 at 03:56:15PM +0200, Niklas Haas wrote:
> From: Niklas Haas <git@haasn.dev>
> 
> The spec specifies x^31 + x^3 + 1 as the polynomial, but the diagram in
> Figure 1-1 omits the +1 offset. The initial implementation was based on
> the diagram, but this is wrong (produces subtly incorrect results).
> ---
>  libavcodec/h274.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/h274.c b/libavcodec/h274.c
> index a69f9411429..fc111cdb50d 100644
> --- a/libavcodec/h274.c
> +++ b/libavcodec/h274.c
> @@ -38,7 +38,7 @@ static void prng_shift(uint32_t *state)
>  {
>      // Primitive polynomial x^31 + x^3 + 1 (modulo 2)
>      uint32_t x = *state;
> -    uint8_t feedback = (x >> 2) ^ (x >> 30);
> +    uint8_t feedback = 1u ^ (x >> 2) ^ (x >> 30);
>      *state = (x << 1) | (feedback & 1u);

where can i find the text describing this that you refer to ?

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
than the original author, trying to rewrite it will not make it better.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] lavc/h274: fix PRNG definition

[Niklas Haas]

On Wed, 27 Sep 2023 21:07:55 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote:
> where can i find the text describing this that you refer to ?

It is in SMPTE RDD 5 (DOI 10.5594/SMPTE.RDD5.2006), you can find it on
the usual places.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] lavc/h274: fix PRNG definition

[Vittorio Giovara]

On Thu, Sep 28, 2023 at 11:08 AM Niklas Haas <ffmpeg@haasn.xyz> wrote:

> On Wed, 27 Sep 2023 21:07:55 +0200 Michael Niedermayer <
> michael@niedermayer.cc> wrote:
> > where can i find the text describing this that you refer to ?
>
> It is in SMPTE RDD 5 (DOI 10.5594/SMPTE.RDD5.2006), you can find it on
> the usual places.
>

Could it maybe be mentioned in the commit log? Or documented in a comment
(not familiar with the file, apologies if it's already so)
-- 
Vittorio
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] lavc/h274: fix PRNG definition

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1236 bytes --]

On Wed, Sep 27, 2023 at 03:56:15PM +0200, Niklas Haas wrote:
> From: Niklas Haas <git@haasn.dev>
> 
> The spec specifies x^31 + x^3 + 1 as the polynomial, but the diagram in
> Figure 1-1 omits the +1 offset. The initial implementation was based on
> the diagram, but this is wrong (produces subtly incorrect results).

the +1 changes "nothing"
if you take the prng with and without the +1 the 2 will produce 2
sequences that are negations of each other

or said different if you start one from 1 and the other from ~1
they will produce the same sequence just 0 and 1 swaped

you can also compute 32 bits at once using this:
(this needs 64bits of the sequence as input though)
not sure how useful it is, but it produces more bits quicker

 static void prng_shift32(uint64_t *state)
 {
     uint64_t x = *state;
     uint64_t y = x ^ x>>3;
     y ^= y>>6;
     y ^= y>>12;
     uint32_t feedback = (x >> 1) ^ (y >> 5) ^ (x >> 29) ^ (x >> 30);
     *state = (x << 32) | feedback;
 }


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] lavc/h274: fix PRNG definition

[Niklas Haas]

On Thu, 28 Sep 2023 20:53:44 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Wed, Sep 27, 2023 at 03:56:15PM +0200, Niklas Haas wrote:
> > From: Niklas Haas <git@haasn.dev>
> > 
> > The spec specifies x^31 + x^3 + 1 as the polynomial, but the diagram in
> > Figure 1-1 omits the +1 offset. The initial implementation was based on
> > the diagram, but this is wrong (produces subtly incorrect results).
> 
> the +1 changes "nothing"
> if you take the prng with and without the +1 the 2 will produce 2
> sequences that are negations of each other
> 
> or said different if you start one from 1 and the other from ~1
> they will produce the same sequence just 0 and 1 swaped
> 
> you can also compute 32 bits at once using this:
> (this needs 64bits of the sequence as input though)
> not sure how useful it is, but it produces more bits quicker
> 
>  static void prng_shift32(uint64_t *state)
>  {
>      uint64_t x = *state;
>      uint64_t y = x ^ x>>3;
>      y ^= y>>6;
>      y ^= y>>12;
>      uint32_t feedback = (x >> 1) ^ (y >> 5) ^ (x >> 29) ^ (x >> 30);
>      *state = (x << 32) | feedback;
>  }

Thanks for the explanation and tip.

In this case, PRNG output is by far not the bottleneck (we only need one
32-bit random value per 16x16 block), so I don't think it needs to be
modified further.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] lavc/h274: fix PRNG definition

[Niklas Haas]

On Thu, 28 Sep 2023 12:03:08 -0400 Vittorio Giovara <vittorio.giovara@gmail.com> wrote:
> On Thu, Sep 28, 2023 at 11:08 AM Niklas Haas <ffmpeg@haasn.xyz> wrote:
> 
> > On Wed, 27 Sep 2023 21:07:55 +0200 Michael Niedermayer <
> > michael@niedermayer.cc> wrote:
> > > where can i find the text describing this that you refer to ?
> >
> > It is in SMPTE RDD 5 (DOI 10.5594/SMPTE.RDD5.2006), you can find it on
> > the usual places.
> >
> 
> Could it maybe be mentioned in the commit log? Or documented in a comment
> (not familiar with the file, apologies if it's already so)

Hi,

it is mentioned in the commit message attached to the initial
implementation of this file, and also linked multiple times throughout
the source code.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".