Closed Bug 1320895 Opened 3 years ago Closed 3 years ago

Incorrect usage of << in dom/media/mediasource/ContainerParser.cpp ?

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: Sylvestre, Assigned: rillian)

References

Details

Attachments

(2 files)

gcc 7.0 considers that the declaration

    size_t data_length = (((*aData)[3] & 0x03) << 11) ||
                         (((*aData)[4] & 0xff) << 3) ||
                         (((*aData)[5] & 0xe0) >> 5);
is probably a mistake:
/root/firefox-gcc-last/dom/media/mediasource/ContainerParser.cpp:587:48: error: '<<' in boolean context, did you mean '<' ? [-Werror=int-in-bool-context]
     size_t data_length = (((*aData)[3] & 0x03) << 11) ||
                          ~~~~~~~~~~~~~~~~~~~~~~^~~~~~
/root/firefox-gcc-last/dom/media/mediasource/ContainerParser.cpp:588:48: error: '<<' in boolean context, did you mean '<' ? [-Werror=int-in-bool-context]
                          (((*aData)[4] & 0xff) << 3) ||
                          ~~~~~~~~~~~~~~~~~~~~~~^~~~~


https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/ContainerParser.cpp?q=dom%2Fmedia%2Fmediasource%2FContainerParser.cpp&redirect_type=direct#587

I won't risk myself in proposing a patch here :)
Guys, what do you think?
Flags: needinfo?(jyavenard)
Flags: needinfo?(giles)
It's unused code at present. But appears to me that || should be replaced by |

:rillian do you want to handle this?
Flags: needinfo?(jyavenard)
Assignee: nobody → giles
Flags: needinfo?(giles)
Comment on attachment 8815886 [details]
Bug 1320895 - Correct ADTS frame size calculation.

https://reviewboard.mozilla.org/r/96688/#review96924
Attachment #8815886 - Flags: review?(gsquelart) → review+
Comment on attachment 8815887 [details]
Bug 1320895 - Test ADTS frame size calculation.

https://reviewboard.mozilla.org/r/96686/#review96926

Thank you for adding this test.
r+ without the printf.

::: dom/media/mediasource/gtest/TestContainerParser.cpp:89
(Diff revision 1)
>    EXPECT_FALSE(NS_SUCCEEDED(parser->IsMediaSegmentPresent(header)))
>      << "Found media segment when there was just a header.";
>    int64_t start = 0;
>    int64_t end = 0;
>    EXPECT_TRUE(NS_FAILED(parser->ParseStartAndEndTimestamps(header, start, end)));
> +  printf("start %llu end %llu\n", start, end);

This debugging printf should probably go.
Attachment #8815887 - Flags: review?(gsquelart) → review+
Comment on attachment 8815887 [details]
Bug 1320895 - Test ADTS frame size calculation.

https://reviewboard.mozilla.org/r/96686/#review96926

> This debugging printf should probably go.

Indeed it should. Thanks for the quick review!
Comment on attachment 8815887 [details]
Bug 1320895 - Test ADTS frame size calculation.

https://reviewboard.mozilla.org/r/96686/#review96934
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff6eea6062d4
Correct ADTS frame size calculation. r=gerald
https://hg.mozilla.org/integration/autoland/rev/928ed9aab0b9
Test ADTS frame size calculation. r=gerald
https://hg.mozilla.org/mozilla-central/rev/ff6eea6062d4
https://hg.mozilla.org/mozilla-central/rev/928ed9aab0b9
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.