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

Status: RESOLVED FIXED

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

Description

[Sylvestre Ledru [:Sylvestre]]

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 :)

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Guys, what do you think?

Comment 2

[Jean-Yves Avenard [:jya]]

It's unused code at present. But appears to me that || should be replaced by |

:rillian do you want to handle this?

Comment 3

[Ralph Giles (:rillian)]

Attachment: Bug 1320895 - Correct ADTS frame size calculation.

This should be bitwise OR rather than logical OR, which just
returns 1. This code has clearly never worked, but it isn't
currently called.

Review commit: https://reviewboard.mozilla.org/r/96688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96688/

Comment 4

[Ralph Giles (:rillian)]

Attachment: Bug 1320895 - Test ADTS frame size calculation.

We don't generate timestamps for ADTS but we can verify
the Init Segment Range and Media Segment Range returned
by the parser match the frame size declared in the
Init Segment header.

Review commit: https://reviewboard.mozilla.org/r/96686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96686/

Comment 5

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

Comment on attachment 8815886 [details]
Bug 1320895 - Correct ADTS frame size calculation.

https://reviewboard.mozilla.org/r/96688/#review96924

Comment 6

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

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.

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8815887 [details]
Bug 1320895 - Test ADTS frame size calculation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96686/diff/1-2/

Comment 8

[Ralph Giles (:rillian)]

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 9

[Ralph Giles (:rillian)]

Comment on attachment 8815887 [details]
Bug 1320895 - Test ADTS frame size calculation.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ff6eea6062d4
https://hg.mozilla.org/mozilla-central/rev/928ed9aab0b9