Closed Bug 1210319 Opened 4 years ago Closed 4 years ago

Remove unused parts of libstagefright

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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
Part 1: Removed unused/useless code.
Assignee: nobody → gsquelart
Attachment #8692753 - Flags: review?(giles)
Attached patch 1210319-p2-cppization.patch (obsolete) — Splinter Review
Part 2: Minor interface clean-up.
Attachment #8692754 - Flags: review?(giles)
Part 3: Lowered useless log level.
Attachment #8692755 - Flags: review?(jyavenard)
Attachment #8692755 - Flags: review?(jyavenard) → review+
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.
Attachment #8692753 - Flags: review?(giles) → review+
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.
Attachment #8692754 - Flags: review?(giles) → review+
(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!
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.
Attachment #8692754 - Attachment is obsolete: true
Attachment #8693985 - Flags: review?(giles)
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.
Attachment #8693985 - Flags: review?(giles) → review+
(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.
You need to log in before you can comment on or make changes to this bug.