1156689 - Abstract libstagefright dependency into a MP4Metadata object

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

This object would follow the new MediaDataDemuxer API as described in bug 1154164

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Part1. Remove mp4_demuxer::TrackType

Remove mp4_demuxer::TrackType

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Part2. Remove MP4 Index's libstagefright dependency

Remove dependency on libstagefright's object type in mp4_demuxer objects

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Part3. Add TrackInfo virtual destructor

Add virtual destructor to TrackInfo

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Part4. Create virtual getters

Add virtual getters. Allows to get rid of a static_cast

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Part5. Add MP4Metadata class

Add MP4Metadata class

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Part6. Add ResourceStream class

Add ResourceStream object. We can't use MP4Stream as it's not really blocking and we want to get rid of the InvokeAndRetry hack. This is basically what MP4Stream used to be, with some enhancement such as pinning the underlying resource while in use

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Part7. Add MoofParser::HasMetadata method

Add MoofParser::HasMetadata method. We don't want to be blocking while attempting to read the metadata with libstagefright on a partial init segment. This will allow to get rid of the ContainerParser class

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Part8. Use new MoofParser::HasMetadata in MP4Metadata

Use MoofParser::HasMetadata() in the MP4Metadata to ensure we do not block attempting to read partial init segment.

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Part9. Remove unused headers

remove stray header. not sure why it was ever added

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Part10. Remove static_cast on TrackInfo objects

Remove static_cast on TrackInfo objects.

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: Part11. Add TrackInfo Clone() and copy constructors

Add Clone() and copy constructor to TrackInfo's classes.

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: Part11. Add TrackInfo Clone() and copy constructors

Bad rebase.. fixed

Comment 13

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8596359 [details] [diff] [review]
Part2. Remove MP4 Index's libstagefright dependency

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

::: media/libstagefright/binding/mp4_demuxer.cpp
@@ +91,5 @@
>  }
>  
> +static void
> +ConvertIndex(nsTArray<Index::Indice>& aDest,
> +             const stagefright::Vector<stagefright::MediaSource::Indice>& aIndex)

Should return nsTArray see https://bugzilla.mozilla.org/show_bug.cgi?id=982212#c20

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8596361 [details] [diff] [review]
Part3. Add TrackInfo virtual destructor

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

::: dom/media/MediaInfo.h
@@ +93,5 @@
>      return mType;
>    }
>    bool virtual IsValid() const = 0;
>  
> +  virtual ~TrackInfo()

Is this object refcounted? If not, please add MOZ_COUNT_CTOR/DTOR.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8596363 [details] [diff] [review]
Part4. Create virtual getters

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

::: dom/media/MediaInfo.h
@@ +79,5 @@
>    int64_t mDuration;
>    int64_t mMediaTime;
>    CryptoTrack mCrypto;
>  
> +  virtual AudioInfo* GetAudioInfo()

Maybe call these GetAs...() to imply you're not getting something from them, but rather converting their type.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8596453 [details] [diff] [review]
Part11. Add TrackInfo Clone() and copy constructors

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

Are the extraData fields refcounted? If so, should you deep copy them? Or is copying its refcounted pointer sufficient?

Comment 17

[Jean-Yves Avenard [:jya]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #13)
> Comment on attachment 8596359 [details] [diff] [review]
> Part2. Remove MP4 Index's libstagefright dependency
> 
> Review of attachment 8596359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libstagefright/binding/mp4_demuxer.cpp
> @@ +91,5 @@
> >  }
> >  
> > +static void
> > +ConvertIndex(nsTArray<Index::Indice>& aDest,
> > +             const stagefright::Vector<stagefright::MediaSource::Indice>& aIndex)
> 
> Should return nsTArray see
> https://bugzilla.mozilla.org/show_bug.cgi?id=982212#c20

That code is removed in part5, where we want to return both a boolean and an array, I use the out parameter there.

Comment 18

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #16)
> Are the extraData fields refcounted? If so, should you deep copy them? Or is
> copying its refcounted pointer sufficient?

shallow copy is sufficient, they aren't supposed to change.

I would have made them const if such possibility with ref counted objects existed.

Comment 19

[Jean-Yves Avenard [:jya]]

Rename and the original intent of this bug will continue in bug 1159027

Comment 20

[Jean-Yves Avenard [:jya]]

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

Comment 21

[Jean-Yves Avenard [:jya]]

Forgot to add Stream.h to hg.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=95b2a37f4c41

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c101c115a2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e07e2b10a78
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2ed413a2048
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b6149e01e96
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a5d030152c
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfb189bc9368
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5282779b0ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/69d8168282fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/be9974876342
https://hg.mozilla.org/integration/mozilla-inbound/rev/005828e55212
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b77e03a4788

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c101c115a2f
https://hg.mozilla.org/mozilla-central/rev/5e07e2b10a78
https://hg.mozilla.org/mozilla-central/rev/d2ed413a2048
https://hg.mozilla.org/mozilla-central/rev/4b6149e01e96
https://hg.mozilla.org/mozilla-central/rev/e6a5d030152c
https://hg.mozilla.org/mozilla-central/rev/cfb189bc9368
https://hg.mozilla.org/mozilla-central/rev/d5282779b0ca
https://hg.mozilla.org/mozilla-central/rev/69d8168282fc
https://hg.mozilla.org/mozilla-central/rev/be9974876342
https://hg.mozilla.org/mozilla-central/rev/005828e55212
https://hg.mozilla.org/mozilla-central/rev/4b77e03a4788

Comment 24

[Daniel Holbert [:dholbert]]

The TrackInfo subclasses' overrides of Clone() (from part11) and GetAsAudioInfo()/GetAsVideoInfo() (from part4) were missing the "override" keyword. This causes a build warning on clang 3.6 & newer -- see bug 1117034 -- & is treated as an error in warnings-as-errors builds.

I pushed a followup to add that keyword, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92f70afc7a26

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92f70afc7a26