Closed Bug 577204 Opened 9 years ago Closed 9 years ago

vp8 warnings about comparison between signed and unsigned

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 608066

People

(Reporter: timeless, Assigned: timeless)

References

()

Details

Attachments

(1 file, 3 obsolete files)

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
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #456233 - Flags: review?(chris)
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/.
Attachment #456233 - Flags: review?(chris) → review-
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.
Assignee: timeless → michaelkohler
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #456233 - Attachment is obsolete: true
Attachment #456276 - Flags: review?(tterribe)
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.
Attachment #456276 - Flags: review?(tterribe) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
timeless: this means you just need to fix the NELEMENTS thing derf said.

.. sorry for the bugspam ..
Attachment #456276 - Attachment is obsolete: true
timeless: could you provide an updated patch with comment 5's changes?
Assignee: michaelkohler → timeless
Attached patch Patch v4Splinter Review
Attachment #456293 - Attachment is obsolete: true
Attachment #488728 - Flags: review?(tterribe)
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.
Attachment #488728 - Flags: review?(tterribe) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 608066
You need to log in before you can comment on or make changes to this bug.