Closed Bug 1153730 Opened 5 years ago Closed 4 years ago

libstagefright/MPEG4Extractor.cpp:2059:29: warning: comparison of constant '2' with boolean expression is always false [-Wbool-compare]

Categories

(Core :: Audio/Video: Playback, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1219475
Tracking Status
firefox40 --- affected

People

(Reporter: dholbert, Unassigned)

References

(Blocks 1 open bug)

Details

Build warning, with clang 3.6 (probably other compilers as well):
{
media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp:2059:29: warning: comparison of constant '2' with boolean expression is always false [-Wbool-compare]
          if (!sap || saptype > 2) {
                              ^
}

This is in code that we imported in bug 908503.

Code snippet, before the warning:
> 2046         uint32_t d1, d2, d3;
> 2047 
> 2048         if (!mDataSource->getUInt32(offset, &d1) ||     // size
> 2049             !mDataSource->getUInt32(offset + 4, &d2) || // duration
> 2050             !mDataSource->getUInt32(offset + 8, &d3)) { // flags
> 2051             return ERROR_MALFORMED;
> 2052         }
[...]
> 2058         bool saptype = d3 >> 28;
> 2059         if (!sap || saptype > 2) {
> 2060             ALOGW("not a stream access point, or unsupported type");
> 2061         }

Basically: in the "saptype > 2" conversion, saptype is promoted to a uint32_t. A bool --> uint32_t promotion can only produce the values 0 or 1.  So, even if saptype's source-data was something greater than 2, that information is lost as soon as it's crammed into a bool. (and then promoted back out of the bool later on for the comparison)

I'm guessing it's not that important of a check, since the only result is some logging. Still, there's no point in performing a false-100%-of-the-time-by-definition comparison.

Someone (maybe me) should probably file this upstream. Filing it on our end for now, to have a record of it here, since we have this code in our tree.
(In reply to Daniel Holbert [:dholbert] from comment #0)
> Basically: in the "saptype > 2" conversion, saptype is promoted to a uint32_t

(er, promoted to an int, I think, to match the type of the literal '2'. Anyway, the point remains.)
Component: Audio/Video → Audio/Video: Playback
From the specs this SAP-type field is a 3-bit value at bits 28-30 in 'd3', so we should probably shift&mask it into an unsigned integer type.

SAP types 1 and 2 are known (and a 0-value is valid as well here), so the test |saptype > 2| is correct, assuming we fix the type.

The only problem would be that a file with a SAP type of 3 would not show the warning.

I'll investigate a bit further to ensure my assumptions are correct, before proposing a patch.
Assignee: nobody → gsquelart
Getting fixed in bug 1219475.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1219475
You need to log in before you can comment on or make changes to this bug.