1350178 - Get stagefright indices only when rust parser is off or TestMode is on.

Status: RESOLVED FIXED

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

Description

[Alfredo Yang (:alfredo)]

Indice table could occupy significant memory, we don't need to keep both if TestMode is off.

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: Bug 1350178 - Get stagefright indices only when rust parser is off or TestMode is on for saving memory usage.

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

Comment 2

[Matthew Gregan [:kinetik]]

Comment on attachment 8850797 [details]
Bug 1350178 - Get stagefright indices only when rust parser is off or TestMode is on for saving memory usage.

https://reviewboard.mozilla.org/r/123316/#review125702

::: media/libstagefright/binding/MP4Metadata.cpp:426
(Diff revision 1)
>  mozilla::UniquePtr<IndiceWrapper>
>  MP4Metadata::GetTrackIndice(mozilla::TrackID aTrackID)
>  {
>    FallibleTArray<Index::Indice> indiceSF;
> -  if(!mStagefright->ReadTrackIndex(indiceSF, aTrackID)) {
> +  if ((mRustTestMode || !mPreferRust) &&
> +       mStagefright->ReadTrackIndex(indiceSF, aTrackID)) {

Originally this returned nullptr if ReadTrackIndex failed, now it returns nullptr if it succeeds (as long as `(mRustTestMode || !mPreferRust)` is true).  Was that intentional?

Comment 3

[Alfredo Yang (:alfredo)]

Comment on attachment 8850797 [details]
Bug 1350178 - Get stagefright indices only when rust parser is off or TestMode is on for saving memory usage.

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

Comment 4

[Matthew Gregan [:kinetik]]

Comment on attachment 8850797 [details]
Bug 1350178 - Get stagefright indices only when rust parser is off or TestMode is on for saving memory usage.

https://reviewboard.mozilla.org/r/123316/#review125722

::: media/libstagefright/binding/MP4Metadata.cpp:425
(Diff revision 2)
>  
>  mozilla::UniquePtr<IndiceWrapper>
>  MP4Metadata::GetTrackIndice(mozilla::TrackID aTrackID)
>  {
>    FallibleTArray<Index::Indice> indiceSF;
> -  if(!mStagefright->ReadTrackIndex(indiceSF, aTrackID)) {
> +  if ((mRustTestMode || !mPreferRust) &&

Can you make that `(!mPreferRust || mRustTestMode)` just for symmetry with the block below that fetches the Rust indices? It makes it slightly easier to spot the difference.

Comment 5

[Alfredo Yang (:alfredo)]

Comment on attachment 8850797 [details]
Bug 1350178 - Get stagefright indices only when rust parser is off or TestMode is on for saving memory usage.

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

Comment 6

[Alfredo Yang (:alfredo)]

Comment on attachment 8850797 [details]
Bug 1350178 - Get stagefright indices only when rust parser is off or TestMode is on for saving memory usage.

https://reviewboard.mozilla.org/r/123316/#review125722

> Can you make that `(!mPreferRust || mRustTestMode)` just for symmetry with the block below that fetches the Rust indices? It makes it slightly easier to spot the difference.

Sure, I'll fixed it.

Comment 7

[Alfredo Yang (:alfredo)]

Comment on attachment 8850797 [details]
Bug 1350178 - Get stagefright indices only when rust parser is off or TestMode is on for saving memory usage.

https://reviewboard.mozilla.org/r/123316/#review125758

::: media/libstagefright/binding/MP4Metadata.cpp:425
(Diff revision 2)
>  
>  mozilla::UniquePtr<IndiceWrapper>
>  MP4Metadata::GetTrackIndice(mozilla::TrackID aTrackID)
>  {
>    FallibleTArray<Index::Indice> indiceSF;
> -  if(!mStagefright->ReadTrackIndex(indiceSF, aTrackID)) {
> +  if ((mRustTestMode || !mPreferRust) &&

Sure. I'll fixed it.

Comment 8

[Alfredo Yang (:alfredo)]

Comment on attachment 8850797 [details]
Bug 1350178 - Get stagefright indices only when rust parser is off or TestMode is on for saving memory usage.

https://reviewboard.mozilla.org/r/123316/#review125702

> Originally this returned nullptr if ReadTrackIndex failed, now it returns nullptr if it succeeds (as long as `(mRustTestMode || !mPreferRust)` is true).  Was that intentional?

Urg... it is a stupid mistake. Thanks for catch it!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/603c4b701a67
Get stagefright indices only when rust parser is off or TestMode is on for saving memory usage. r=kinetik

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/603c4b701a67