Closed Bug 577204 Opened 9 years ago Closed 9 years ago
vp8 warnings about comparison between signed and unsigned
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
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
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 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-
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
Comment on attachment 488728 [details] [diff] [review] Patch v4 > -#define NELEMENTS(x) (sizeof(x)/sizeof(x)) > +#define NELEMENTS(x) (int)(sizeof(x)/sizeof(x)) 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.