[FFmpeg-devel] avcodec/texturedspenc: Fix indexing in color distribution determination

Submitted by Michael Niedermayer on Jan. 1, 2017, 11:28 p.m.

Details

Message ID 20170101232833.24472-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Jan. 1, 2017, 11:28 p.m.
Fixes CID1396405

Untested except fate which does not test this, please someone test this

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/texturedspenc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

James Almer Jan. 2, 2017, 1 a.m.
On 1/1/2017 8:28 PM, Michael Niedermayer wrote:
> Fixes CID1396405
> 
> Untested except fate which does not test this, please someone test this

The encoder seems to need libsnappy, so no FATE test can be added.

CCing Vittorio since he wrote this. He may be able to test and/or
review.

> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/texturedspenc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/texturedspenc.c b/libavcodec/texturedspenc.c
> index 8b2863033b..5171c69d82 100644
> --- a/libavcodec/texturedspenc.c
> +++ b/libavcodec/texturedspenc.c
> @@ -255,11 +255,11 @@ static void optimize_colors(const uint8_t *block, ptrdiff_t stride,
>  
>          muv = minv = maxv = bp[0];
>          for (y = 0; y < 4; y++) {
> -            for (x = 4; x < 4; x += 4) {
> +            for (x = 0; x < 4; x++) {
>                  muv += bp[x * 4 + y * stride];
> -                if (bp[x] < minv)
> +                if (bp[x * 4 + y * stride] < minv)
>                      minv = bp[x * 4 + y * stride];
> -                else if (bp[x] > maxv)
> +                else if (bp[x * 4 + y * stride] > maxv)
>                      maxv = bp[x * 4 + y * stride];
>              }
>          }
>
Vittorio Giovara Jan. 3, 2017, 1:15 p.m.
On Mon, Jan 2, 2017 at 2:00 AM, James Almer <jamrial@gmail.com> wrote:
> On 1/1/2017 8:28 PM, Michael Niedermayer wrote:
>> Fixes CID1396405

What is the CID about?

>> Untested except fate which does not test this, please someone test this
>
> The encoder seems to need libsnappy, so no FATE test can be added.

Only the HAP encoder needs libsnappy, this module has no external dependencies.
A possible way to test this is to encode a HAP video using -format
option and hap_alpha or hap_q modes and then decode it back with the
internal decoder.

> CCing Vittorio since he wrote this. He may be able to test and/or
> review.

Thanks for the CC.
Michael Niedermayer Jan. 4, 2017, 12:06 a.m.
On Tue, Jan 03, 2017 at 02:15:20PM +0100, Vittorio Giovara wrote:
> On Mon, Jan 2, 2017 at 2:00 AM, James Almer <jamrial@gmail.com> wrote:
> > On 1/1/2017 8:28 PM, Michael Niedermayer wrote:
> >> Fixes CID1396405
> 
> What is the CID about?

sorry for not quoting this prviously, it isnt very interresting.
my patch is based on just this and guesswork, which is why it should
be tested.

257        for (y = 0; y < 4; y++) {
   assignment: Assigning: x = 4.
   const: At condition x < 4, the value of x must be equal to 4.
   dead_error_condition: The condition x < 4 cannot be true.
258            for (x = 4; x < 4; x += 4) {
   CID 1396405 (#1 of 1): Logically dead code (DEADCODE)dead_error_begin: Execution cannot reach this statement: muv += bp[x * 4 + y * stride];.
259                muv += bp[x * 4 + y * stride];
260                if (bp[x] < minv)
261                    minv = bp[x * 4 + y * stride];
262                else if (bp[x] > maxv)
263                    maxv = bp[x * 4 + y * stride];
264            }
265        }


[...]
Vittorio Giovara Jan. 4, 2017, 10:52 p.m.
On Wed, Jan 4, 2017 at 1:06 AM, Michael Niedermayer
<michael@niedermayer.cc> wrote:
> On Tue, Jan 03, 2017 at 02:15:20PM +0100, Vittorio Giovara wrote:
>> On Mon, Jan 2, 2017 at 2:00 AM, James Almer <jamrial@gmail.com> wrote:
>> > On 1/1/2017 8:28 PM, Michael Niedermayer wrote:
>> >> Fixes CID1396405
>>
>> What is the CID about?
>
> sorry for not quoting this prviously, it isnt very interresting.
> my patch is based on just this and guesswork, which is why it should
> be tested.
>
> 257        for (y = 0; y < 4; y++) {
>    assignment: Assigning: x = 4.
>    const: At condition x < 4, the value of x must be equal to 4.
>    dead_error_condition: The condition x < 4 cannot be true.
> 258            for (x = 4; x < 4; x += 4) {
>    CID 1396405 (#1 of 1): Logically dead code (DEADCODE)dead_error_begin: Execution cannot reach this statement: muv += bp[x * 4 + y * stride];.

Ah, yes that seems wrong, probably a leftover of the code conversion
(from https://github.com/nothings/stb/blob/master/stb_dxt.h#L293-L298)
I believe it should be sufficient to do `for (x = 0; x < 4; x++)` but
I won't be able to test that for a while I'm afraid

Patch hide | download patch | download mbox

diff --git a/libavcodec/texturedspenc.c b/libavcodec/texturedspenc.c
index 8b2863033b..5171c69d82 100644
--- a/libavcodec/texturedspenc.c
+++ b/libavcodec/texturedspenc.c
@@ -255,11 +255,11 @@  static void optimize_colors(const uint8_t *block, ptrdiff_t stride,
 
         muv = minv = maxv = bp[0];
         for (y = 0; y < 4; y++) {
-            for (x = 4; x < 4; x += 4) {
+            for (x = 0; x < 4; x++) {
                 muv += bp[x * 4 + y * stride];
-                if (bp[x] < minv)
+                if (bp[x * 4 + y * stride] < minv)
                     minv = bp[x * 4 + y * stride];
-                else if (bp[x] > maxv)
+                else if (bp[x * 4 + y * stride] > maxv)
                     maxv = bp[x * 4 + y * stride];
             }
         }