577204 - vp8 warnings about comparison between signed and unsigned

Status: RESOLVED DUPLICATE of bug 608066

(Core :: Audio/Video, defect)

Description

[timeless]

media/libvpx/vp8/

common/loopfilter.c: In function ‘vp8_loop_filter_frame’:
common/loopfilter.c:354: warning: comparison between signed and unsigned
common/loopfilter.c: In function ‘vp8_loop_filter_frame_yonly’:
common/loopfilter.c:459: warning: comparison between signed and unsigned
common/loopfilter.c: In function ‘vp8_loop_filter_partial_frame’:
common/loopfilter.c:567: warning: comparison between signed and unsigned
vp8_dx_iface.c: In function ‘vp8_validate_mmaps’:
vp8_dx_iface.c:121: warning: comparison between signed and unsigned
vp8_dx_iface.c: In function ‘vp8_init_ctx’:
vp8_dx_iface.c:158: warning: comparison between signed and unsigned
vp8_dx_iface.c: In function ‘mmap_lkup’:
vp8_dx_iface.c:177: warning: comparison between signed and unsigned
vp8_dx_iface.c:178: warning: comparison between signed and unsigned
vp8_dx_iface.c: In function ‘vp8_decode’:
vp8_dx_iface.c:346: warning: comparison between signed and unsigned
vp8_dx_iface.c: In function ‘vp8_xma_set_mmap’:
vp8_dx_iface.c:535: warning: comparison between signed and unsigned

Comment 1

[timeless]

Attachment: patch

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 456233 [details] [diff] [review]
patch

Thanks! You need to include the changes as a patch in media/libvpx/ directory, and change update.sh so that the patch is applied when libvpx is updated from upstream. Getting the changes upstream would be nice too.

Tim Terriberry has taken ownership of media/libvpx/, so you should ask him for review for patches to media/libvpx/.

Comment 3

[timeless]

i've filed a bug for upstream:
http://code.google.com/p/webm/issues/detail?id=121

the instructions for downstream are too complicated for me.

Comment 4

[Michael Kohler [:mkohler]]

Attachment: Patch v2

Chris: do you mean like that? I think bringing this change not upstream would be mean, so I'll sign the agreement today and push the change to review for the libvpx guys.

Comment 5

[Timothy B. Terriberry (:derf)]

Comment on attachment 456276 [details] [diff] [review]
Patch v2

>+-    int i;
>++    unsigned int i;
>+     vpx_codec_err_t res = VPX_CODEC_OK;
>+ 
>+     for (i = 0; i < NELEMENTS(vp8_mem_req_segs) - 1; i++)

I would suggest changing the NELEMENTS macro to cast its result to int instead. There's no situation in which the actual number of elements wouldn't fit in an int, and this both avoids promoting i to a size_t on platforms where they are different sizes and adding a new signed/unsigned mismatch on platforms where sizeof does not return a size_t like it's supposed to.

This patch should also include the diff for the actual application of the patch to our local copy of libvpx, as update.sh is only run when importing a new version of the library, not on every build.

Comment 6

[Michael Kohler [:mkohler]]

Attachment: Patch v3

timeless: this means you just need to fix the NELEMENTS thing derf said.

.. sorry for the bugspam ..

Comment 7

[Michael Kohler [:mkohler]]

timeless: could you provide an updated patch with comment 5's changes?

Comment 8

[timeless]

Attachment: Patch v4

Comment 9

[Timothy B. Terriberry (:derf)]

Comment on attachment 488728 [details] [diff] [review]
Patch v4

> -#define NELEMENTS(x) (sizeof(x)/sizeof(x[0]))
> +#define NELEMENTS(x) (int)(sizeof(x)/sizeof(x[0]))

This sort of thing makes me nervous. The precedence of typecasting is high enough that I don't see how the lack of parentheses around the whole macro expression could cause problems, but that may not be true if someone edits the macro later.

With that extra set of parentheses, r=me.

N.B. that the patch in bug 608066 already includes these signed-unsigned comparison fixes, along with many other warning fixes (see upstream changeset https://review.webmproject.org/1026). If we're going to take one of these, I'd prefer to take that one, as it has many other benefits.