1210319 - Remove unused parts of libstagefright

Status: RESOLVED FIXED

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

Description

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

Only a subset of libstagefright is actually used.

The remainder is just sitting there, attracting false alarms, sucking up maintenance time.

WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc3294931144

Comment 1

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

Attachment: 1210319-p1-prune-stagefright.patch

Part 1: Removed unused/useless code.

Comment 2

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

Attachment: 1210319-p2-cppization.patch

Part 2: Minor interface clean-up.

Comment 3

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

Attachment: 1210319-p3-reduce-logging.patch

Part 3: Lowered useless log level.

Comment 4

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f49891fd2aca

Comment 5

[Ralph Giles (:rillian)]

Comment on attachment 8692753 [details] [diff] [review]
1210319-p1-prune-stagefright.patch

Review of attachment 8692753 [details] [diff] [review]:
-----------------------------------------------------------------

Yay! I'm going to assume everything still works.

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8692754 [details] [diff] [review]
1210319-p2-cppization.patch

Review of attachment 8692754 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/frameworks/av/media/libstagefright/include/MPEG4Extractor.h
@@ +123,5 @@
> +                mDefaultSampleSize(0), mDefaultSampleFlags(0)
> +        {
> +            mFlags[0] = 0;
> +            mFlags[1] = 0;
> +            mFlags[2] = 0;

Would be cleaner with

  mFlags = {0}; 

or

  PodZero(mFlags, ArrayLength(mFlags));

Up to you though.

Comment 7

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

(In reply to Ralph Giles (:rillian) from comment #6)
> Comment on attachment 8692754 [details] [diff] [review]
> 1210319-p2-cppization.patch
> 
> Review of attachment 8692754 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> media/libstagefright/frameworks/av/media/libstagefright/include/
> MPEG4Extractor.h
> @@ +123,5 @@
> > +                mDefaultSampleSize(0), mDefaultSampleFlags(0)
> > +        {
> > +            mFlags[0] = 0;
> > +            mFlags[1] = 0;
> > +            mFlags[2] = 0;
> 
> Would be cleaner with
> 
>   mFlags = {0}; 
> 
> or
> 
>   PodZero(mFlags, ArrayLength(mFlags));
> 
> Up to you though.

Good point, thank you.

I tried to member-initialize mFlags({0}) which seemed the cleanest, but member-initializing arrays is not valid C++:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b8dffddae72

Anyway, upon further examination, this structure was actually only used by code removed in the first patch, and 'trex' parsing doesn't even check for validity, so I'll just remove the whole thing -- Less surface area to attract false alarms!

Comment 8

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

Attachment: 1210319-p2-cppization.patch v2

Part 2, v2: Minor intf clean-up, RIP trex.

Made some class interfaces a bit more "C++11"-ish, to protect against some
possible issues.
Also removed 'trex', which was only used by code removed in previous patch.

Comment 9

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

Try with all gtests, and dom/media tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c35c16e5cfe8

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8693985 [details] [diff] [review]
1210319-p2-cppization.patch v2

Review of attachment 8693985 [details] [diff] [review]:
-----------------------------------------------------------------

Even better, thanks.

I still find it weird how one has to delete ctors in both the declaration and the implementation.

Comment 11

[Ralph Giles (:rillian)]

(In reply to Ralph Giles (:rillian) from comment #10)

> I still find it weird how one has to delete ctors in both the declaration
> and the implementation.

Nevermind, I was confused.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/571d6fc4fcdd
https://hg.mozilla.org/integration/mozilla-inbound/rev/cddca26367a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e68f234636f

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/571d6fc4fcdd
https://hg.mozilla.org/integration/mozilla-inbound/rev/cddca26367a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e68f234636f

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/571d6fc4fcdd
https://hg.mozilla.org/mozilla-central/rev/cddca26367a2
https://hg.mozilla.org/mozilla-central/rev/9e68f234636f