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

Status: RESOLVED DUPLICATE of bug 1219475

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

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

(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.)

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Getting fixed in bug 1219475.