Closed Bug 1156689 Opened 9 years ago Closed 9 years ago

Abstract libstagefright dependency into a MP4Metadata object

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(11 files, 1 obsolete file)

17.87 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
10.26 KB, patch
ajones
: review+
Details | Diff | Splinter Review
818 bytes, patch
cpearce
: review+
Details | Diff | Splinter Review
3.41 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
27.09 KB, patch
ajones
: review+
Details | Diff | Splinter Review
5.25 KB, patch
ajones
: review+
Details | Diff | Splinter Review
2.25 KB, patch
ajones
: review+
Details | Diff | Splinter Review
3.50 KB, patch
ajones
: review+
Details | Diff | Splinter Review
925 bytes, patch
ajones
: review+
Details | Diff | Splinter Review
1.99 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
3.98 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
This object would follow the new MediaDataDemuxer API as described in bug 1154164
Remove mp4_demuxer::TrackType
Attachment #8596358 - Flags: review?(cpearce)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Remove dependency on libstagefright's object type in mp4_demuxer objects
Attachment #8596359 - Flags: review?(ajones)
Add virtual destructor to TrackInfo
Attachment #8596361 - Flags: review?(cpearce)
Add virtual getters. Allows to get rid of a static_cast
Attachment #8596363 - Flags: review?(cpearce)
Add MP4Metadata class
Attachment #8596366 - Flags: review?(ajones)
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
Attachment #8596368 - Flags: review?(ajones)
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
Attachment #8596370 - Flags: review?(ajones)
Use MoofParser::HasMetadata() in the MP4Metadata to ensure we do not block attempting to read partial init segment.
Attachment #8596371 - Flags: review?(ajones)
remove stray header. not sure why it was ever added
Attachment #8596372 - Flags: review?(ajones)
Remove static_cast on TrackInfo objects.
Attachment #8596374 - Flags: review?(cpearce)
Add Clone() and copy constructor to TrackInfo's classes.
Attachment #8596375 - Flags: review?(cpearce)
Bad rebase.. fixed
Attachment #8596453 - Flags: review?(cpearce)
Attachment #8596375 - Attachment is obsolete: true
Attachment #8596375 - Flags: review?(cpearce)
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
Attachment #8596359 - Flags: review?(ajones) → review+
Attachment #8596366 - Flags: review?(ajones) → review+
Attachment #8596368 - Flags: review?(ajones) → review+
Attachment #8596370 - Flags: review?(ajones) → review+
Attachment #8596371 - Flags: review?(ajones) → review+
Attachment #8596372 - Flags: review?(ajones) → review+
Attachment #8596358 - Flags: review?(cpearce) → review+
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.
Attachment #8596361 - Flags: review?(cpearce) → review+
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.
Attachment #8596363 - Flags: review?(cpearce) → review+
Attachment #8596374 - Flags: review?(cpearce) → review+
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?
Attachment #8596453 - Flags: review?(cpearce) → review+
(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.
(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.
Summary: Create new MP4Demuxer, a MediaDataDemuxer object → Abstract libstagefright dependency into a MP4Metadata object
Blocks: 1159027
Rename and the original intent of this bug will continue in bug 1159027
No longer blocks: 1154164
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
Blocks: 1161350
Depends on: 1171067
You need to log in before you can comment on or make changes to this bug.