Closed Bug 1415809 Opened 2 years ago Closed 2 years ago

Remove MP4MetadataStagefright from MP4Metadata

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: ayang, Assigned: ayang)

References

Details

Attachments

(6 files)

No description provided.
Assignee: nobody → ayang
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.
Attachment #8926783 - Flags: review?(kinetik) → review+
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.
Attachment #8926782 - Flags: review?(kinetik) → review+
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.
(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?
Flags: needinfo?(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.
Flags: needinfo?(kinetik)
(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 on attachment 8927238 [details]
Bug 1415809 - remove stagefright stuff from DecoderData.

https://reviewboard.mozilla.org/r/198532/#review203878
Attachment #8927238 - Flags: review?(kinetik) → review+
Comment on attachment 8927237 [details]
Bug 1415809 - rename MP4MetadataRust to MP4Metadata.

https://reviewboard.mozilla.org/r/198530/#review203880
Attachment #8927237 - Flags: review?(kinetik) → review+
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.
Attachment #8927754 - Flags: review?(kinetik) → review+
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?
Attachment #8927755 - Flags: review?(kinetik) → review+
Blocks: 1417011
Pushed by ayang@mozilla.com:
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
Depends on: 1448762
You need to log in before you can comment on or make changes to this bug.