1415809 - Remove MP4MetadataStagefright from MP4Metadata

Status: RESOLVED FIXED

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

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: Bug 1415809 - remove MP4MetadataStagefright.

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

Comment 2

[Alfredo Yang (:alfredo)]

Attachment: Bug 1415809 - return error when parsing fails.

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

Comment 3

[Matthew Gregan [:kinetik]]

Comment on attachment 8926783 [details]
Bug 1415809 - return error when parsing fails.

https://reviewboard.mozilla.org/r/198030/#review203482

::: media/libstagefright/binding/MP4Metadata.cpp:306
(Diff revision 1)
> +MP4MetadataRust::~MP4MetadataRust()
> +{
> +}
>  
> +nsresult
> +MP4MetadataRust::Init()

We should probably move the contents of MP4MetadataRust back up into MP4Metadata, which is how it was (with the Stagefright implementation) before we made the Stagefright/Rust split.

Then this Init() could just be MP4Metadata::Parse.

::: media/libstagefright/binding/MP4Metadata.cpp:314
(Diff revision 1)
>    Telemetry::Accumulate(Telemetry::MEDIA_RUST_MP4PARSE_SUCCESS,
>                          rv == mp4parse_status_OK);
> -  if (rv != mp4parse_status_OK && rv != mp4parse_status_OOM) {
> +  if (rv != mp4parse_status_OK) {
>      MOZ_LOG(gMP4MetadataLog, LogLevel::Info, ("Rust mp4 parser fails to parse this stream."));
> -    MOZ_ASSERT(rv > 0);
>      Telemetry::Accumulate(Telemetry::MEDIA_RUST_MP4PARSE_ERROR_CODE, rv);

It's probably worth reevaluating whether this Telemetry is worth keeping now we're removing Stagefright.

Comment 4

[Matthew Gregan [:kinetik]]

Comment on attachment 8926782 [details]
Bug 1415809 - remove MP4MetadataStagefright.

https://reviewboard.mozilla.org/r/198028/#review203480

::: media/libstagefright/binding/MP4Metadata.cpp
(Diff revision 1)
>  
> -#include "include/MPEG4Extractor.h"
> -#include "media/stagefright/DataSource.h"
> -#include "media/stagefright/MediaDefs.h"
> -#include "media/stagefright/MediaSource.h"
> -#include "media/stagefright/MetaData.h"

Can we also delete the files now, or does something else depend on them?

::: media/libstagefright/binding/MP4Metadata.cpp:163
(Diff revision 1)
>    aIndice.sync = indice->sync;
>    return true;
>  }
>  
>  MP4Metadata::MP4Metadata(Stream* aSource)
> - : mStagefright(MakeUnique<MP4MetadataStagefright>(aSource))
> + : mRust(MakeUnique<MP4MetadataRust>(aSource))

We can rename mRust to mParser or something at this point.

::: media/libstagefright/binding/MP4Metadata.cpp:226
(Diff revision 1)
>  }
>  
>  bool
>  MP4Metadata::CanSeek() const
>  {
> -  return mStagefright->CanSeek();
> +  // It always returns true in SF.

s/SF/Stagefright/, but once the Stagefright code is gone I'm not sure this comment makes sense out of context, so maybe just remove it.

::: media/libstagefright/binding/MP4Metadata.cpp:235
(Diff revision 1)
>  MP4Metadata::Crypto() const
>  {
> -  MP4Metadata::ResultAndCryptoFile crypto = mStagefright->Crypto();
>    MP4Metadata::ResultAndCryptoFile rustCrypto = mRust->Crypto();
>  
> -  if (MediaPrefs::MediaWarningsAsErrorsStageFrightVsRust()) {
> +  return rustCrypto;

We can remove "rust" from the name of everything now that there's no duplication.  And even move everything from MP4MetadataRust back up into the main class since it was just a wrapper for the two duplicates in Rust/Stagefright.

Comment 5

[Alfredo Yang (:alfredo)]

Comment on attachment 8926783 [details]
Bug 1415809 - return error when parsing fails.

https://reviewboard.mozilla.org/r/198030/#review203482

> We should probably move the contents of MP4MetadataRust back up into MP4Metadata, which is how it was (with the Stagefright implementation) before we made the Stagefright/Rust split.
> 
> Then this Init() could just be MP4Metadata::Parse.

I'll have another patch for that.

Comment 6

[Alfredo Yang (:alfredo)]

(In reply to Matthew Gregan [:kinetik] from comment #4)
> Comment on attachment 8926782 [details]
> Bug 1415809 - remove MP4MetadataStagefright.
> 
> https://reviewboard.mozilla.org/r/198028/#review203480
> 
> ::: media/libstagefright/binding/MP4Metadata.cpp
> (Diff revision 1)
> >  
> > -#include "include/MPEG4Extractor.h"
> > -#include "media/stagefright/DataSource.h"
> > -#include "media/stagefright/MediaDefs.h"
> > -#include "media/stagefright/MediaSource.h"
> > -#include "media/stagefright/MetaData.h"
> 
> Can we also delete the files now, or does something else depend on them?

Will do.

And MP4Metadata needs to be moved to another folder, any suggestion?
I think rust parser should stay under /media like /media/mp4parser?

Comment 7

[Matthew Gregan [:kinetik]]

(In reply to Alfredo Yang (:alfredo) from comment #6)
> And MP4Metadata needs to be moved to another folder, any suggestion?
> I think rust parser should stay under /media like /media/mp4parser?

Maybe dom/media/fmp4 or a new mp4-related directory?  Might be worth asking around for other opinions.  We'll also need to move the other stuff we've added to media/libstagefright/binding - the MoofParser bits and whatever else we added that isn't really part of Stagefright.

Comment 8

[Alfredo Yang (:alfredo)]

(In reply to Matthew Gregan [:kinetik] from comment #4)
> Comment on attachment 8926782 [details]
> Bug 1415809 - remove MP4MetadataStagefright.
> 
> https://reviewboard.mozilla.org/r/198028/#review203480
> 
> ::: media/libstagefright/binding/MP4Metadata.cpp
> (Diff revision 1)
> >  
> > -#include "include/MPEG4Extractor.h"
> > -#include "media/stagefright/DataSource.h"
> > -#include "media/stagefright/MediaDefs.h"
> > -#include "media/stagefright/MediaSource.h"
> > -#include "media/stagefright/MetaData.h"
> 
> Can we also delete the files now, or does something else depend on them?

Yes, there are other files depending on them like DecoderData.cpp.

Comment 9

[Alfredo Yang (:alfredo)]

Attachment: Bug 1415809 - rename MP4MetadataRust to MP4Metadata.

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

Comment 10

[Alfredo Yang (:alfredo)]

Attachment: Bug 1415809 - remove stagefright stuff from DecoderData.

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

Comment 11

[Alfredo Yang (:alfredo)]

Comment on attachment 8926782 [details]
Bug 1415809 - remove MP4MetadataStagefright.

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

Comment 12

[Alfredo Yang (:alfredo)]

Comment on attachment 8926783 [details]
Bug 1415809 - return error when parsing fails.

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

Comment 13

[Matthew Gregan [:kinetik]]

Comment on attachment 8927238 [details]
Bug 1415809 - remove stagefright stuff from DecoderData.

https://reviewboard.mozilla.org/r/198532/#review203878

Comment 14

[Matthew Gregan [:kinetik]]

Comment on attachment 8927237 [details]
Bug 1415809 - rename MP4MetadataRust to MP4Metadata.

https://reviewboard.mozilla.org/r/198530/#review203880

Comment 15

[Alfredo Yang (:alfredo)]

Attachment: Bug 1415809 - update gtest.

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

Comment 16

[Alfredo Yang (:alfredo)]

Attachment: Bug 1415809 - stop building stagefright.

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

Comment 17

[Alfredo Yang (:alfredo)]

Comment on attachment 8926782 [details]
Bug 1415809 - remove MP4MetadataStagefright.

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

Comment 18

[Alfredo Yang (:alfredo)]

Comment on attachment 8926783 [details]
Bug 1415809 - return error when parsing fails.

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

Comment 19

[Alfredo Yang (:alfredo)]

Comment on attachment 8927237 [details]
Bug 1415809 - rename MP4MetadataRust to MP4Metadata.

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

Comment 20

[Alfredo Yang (:alfredo)]

Comment on attachment 8927238 [details]
Bug 1415809 - remove stagefright stuff from DecoderData.

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

Comment 21

[Matthew Gregan [:kinetik]]

Comment on attachment 8927754 [details]
Bug 1415809 - update gtest.

https://reviewboard.mozilla.org/r/199028/#review204296

::: media/libstagefright/gtest/TestParser.cpp:180
(Diff revision 1)
> -  { "test_case_1329061.mov",        0, false, -1,   0,   0, 1,  234567981,
> -                                                            false,   0, false, false, 2 },
> -  { "test_case_1351094.mp4",        0, false, -1,   0,   0, 0, -1, false,   0, true,  true,  0 },
> -};
>  
>  static const TestFileData rustTestFiles[] = {

Can rename this to remove "rust" now.

::: media/libstagefright/gtest/TestParser.cpp:236
(Diff revision 1)
> -  { "test_case_1380468.mp4",        0, false,  0,   0,   0, 0,  0, false,   0, false, false, 0 },
> -  { "test_case_1410565.mp4",        1, true, 0,
> +  { "test_case_1380468.mp4",        false, 0, false,  0,   0,   0, 0,  0, false,   0, false, false, 0 },
> +  { "test_case_1410565.mp4",        false, 0, false, 0, 0, 0, 0, 0, false, 955100, true, true, 2 },    // negative 'timescale'
> -                                           320, 180, 1, 0, false, 955100, true,  true,  2 },
>  };
> +
>  TEST(stagefright_MPEG4Metadata, test_case_mp4)

Remove stagefright from the test names to avoid confusion in the future.

Comment 22

[Matthew Gregan [:kinetik]]

Comment on attachment 8927755 [details]
Bug 1415809 - stop building stagefright.

https://reviewboard.mozilla.org/r/199030/#review204298

::: media/libstagefright/moz.build:84
(Diff revision 1)
> -    'frameworks/av/media/libstagefright/foundation/hexdump.cpp',
> -    'frameworks/av/media/libstagefright/MetaData.cpp',
>      'system/core/libutils/RefBase.cpp',
>      'system/core/libutils/String16.cpp',
>      'system/core/libutils/String8.cpp',
>      'system/core/libutils/VectorImpl.cpp',

Are these still being used by something?

::: media/libstagefright/moz.build:106
(Diff revision 1)
> -    'frameworks/av/media/libstagefright/SampleIterator.cpp',
> -    'frameworks/av/media/libstagefright/SampleTable.cpp',
> -    'frameworks/av/media/libstagefright/Utils.cpp',
>      'system/core/libutils/SharedBuffer.cpp',
>      'system/core/libutils/Static.cpp',
>      'system/core/libutils/Unicode.cpp',

Are these still being used somewhere?

Comment 23

[Alfredo Yang (:alfredo)]

Comment on attachment 8926782 [details]
Bug 1415809 - remove MP4MetadataStagefright.

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

Comment 24

[Alfredo Yang (:alfredo)]

Comment on attachment 8926783 [details]
Bug 1415809 - return error when parsing fails.

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

Comment 25

[Alfredo Yang (:alfredo)]

Comment on attachment 8927237 [details]
Bug 1415809 - rename MP4MetadataRust to MP4Metadata.

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

Comment 26

[Alfredo Yang (:alfredo)]

Comment on attachment 8927238 [details]
Bug 1415809 - remove stagefright stuff from DecoderData.

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

Comment 27

[Alfredo Yang (:alfredo)]

Comment on attachment 8927754 [details]
Bug 1415809 - update gtest.

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

Comment 28

[Alfredo Yang (:alfredo)]

Comment on attachment 8927755 [details]
Bug 1415809 - stop building stagefright.

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

Comment 29

[Alfredo Yang (:alfredo)]

Try looks good.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c844e335c81abad56bde65acd0e7ba7a6fb5a7b

Comment 30

[Alfredo Yang (:alfredo)]

Comment on attachment 8926783 [details]
Bug 1415809 - return error when parsing fails.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198030/diff/4-5/

Comment 31

[Alfredo Yang (:alfredo)]

Comment on attachment 8927237 [details]
Bug 1415809 - rename MP4MetadataRust to MP4Metadata.

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

Comment 32

[Alfredo Yang (:alfredo)]

Comment on attachment 8927238 [details]
Bug 1415809 - remove stagefright stuff from DecoderData.

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

Comment 33

[Alfredo Yang (:alfredo)]

Comment on attachment 8927754 [details]
Bug 1415809 - update gtest.

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

Comment 34

[Alfredo Yang (:alfredo)]

Comment on attachment 8927755 [details]
Bug 1415809 - stop building stagefright.

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

Comment 35

[Alfredo Yang (:alfredo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=297cf19d9895c2d819e2bad8e6724641a3fa5661

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9f764d3cbce
remove MP4MetadataStagefright. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/2dba7ab10fd3
return error when parsing fails. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/af9976dd3777
rename MP4MetadataRust to MP4Metadata. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/80de7f787d2a
remove stagefright stuff from DecoderData. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/a24f3a55b60b
update gtest. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/a51ca572b96c
stop building stagefright. r=kinetik

Comment 37

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b9f764d3cbce
https://hg.mozilla.org/mozilla-central/rev/2dba7ab10fd3
https://hg.mozilla.org/mozilla-central/rev/af9976dd3777
https://hg.mozilla.org/mozilla-central/rev/80de7f787d2a
https://hg.mozilla.org/mozilla-central/rev/a24f3a55b60b
https://hg.mozilla.org/mozilla-central/rev/a51ca572b96c