Create MediaData base class for Audio/VideoData classes

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

Trunk
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
I'd like to create a MediaData class that is a base class of AudioData and VideoData. This will mean we can move some of the duplicate fields of Audio/VideoData up into a common parent class, and will make it much easier to create a generic stream decoder abstraction for use in the new MP4 demuxer backend.
(Assignee)

Comment 1

5 years ago
Created attachment 822068 [details] [diff] [review]
Patch v1

* Create MediaData, base class for Audio/VideoData.
* Moved offset, duration, timestamp, GetEndTime() into MediaData class.
* VideoData objects now have an inherited duration field, rather than an end time.
* All POD fields in *Data are now const.
* Created VideoData::ShallowCopyChangeDuration(VideoData*, duration), which creates a shallow copy of a VideoData with a different duration (since the MediaData::mDuration is now const, it can't be changed). The Fennec backend needs this. This does not copy the underlying image, so it's cheapish.
* I also changed the VideoData* in MediaPluginReader into nsAutoPtr<VideoData>.
* Green: https://tbpl.mozilla.org/?tree=Try&rev=fab79a9e1186
Attachment #822068 - Flags: review?(kinetik)
Comment on attachment 822068 [details] [diff] [review]
Patch v1

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

::: content/media/MediaDecoderReader.h
@@ +100,5 @@
> +
> +  enum Type {
> +    AUDIO_SAMPLES = 0,
> +    VIDEO_FRAME = 1
> +  };

mType doesn't seem to be used.  Can we get rid of this?

@@ +260,5 @@
> +  // aOther's mImage, i.e. the Image is not copied. This function is useful
> +  // in reader backends that can't determine the duration of a VideoData
> +  // until the next frame is decoded, i.e. it's a way to change the const
> +  // duration field on a VideoData.
> +  static VideoData* ShallowCopyChangeDuration(VideoData* aOther,

ShallowCopyUpdateDuration maybe?

::: content/media/plugins/MediaPluginReader.cpp
@@ +256,5 @@
>        mLastVideoFrame = v;
>        continue;
>      }
>  
> +    // Calculate the duration as the timestamp of the current frame minus the 

trailing space
Attachment #822068 - Flags: review?(kinetik) → review+
(Assignee)

Comment 3

5 years ago
(In reply to Matthew Gregan [:kinetik] from comment #2)
> Comment on attachment 822068 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 822068 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaDecoderReader.h
> @@ +100,5 @@
> > +
> > +  enum Type {
> > +    AUDIO_SAMPLES = 0,
> > +    VIDEO_FRAME = 1
> > +  };
> 
> mType doesn't seem to be used.  Can we get rid of this?

I'm using it in a later patch in my queue. I'm just trying to land stuff to reduce the chance of bitrot.

> 
> @@ +260,5 @@
> > +  // aOther's mImage, i.e. the Image is not copied. This function is useful
> > +  // in reader backends that can't determine the duration of a VideoData
> > +  // until the next frame is decoded, i.e. it's a way to change the const
> > +  // duration field on a VideoData.
> > +  static VideoData* ShallowCopyChangeDuration(VideoData* aOther,
> 
> ShallowCopyUpdateDuration maybe?

OK.

> 
> ::: content/media/plugins/MediaPluginReader.cpp
> @@ +256,5 @@
> >        mLastVideoFrame = v;
> >        continue;
> >      }
> >  
> > +    // Calculate the duration as the timestamp of the current frame minus the 
> 
> trailing space

Thanks!
https://hg.mozilla.org/mozilla-central/rev/2fd66c1fac23
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(Assignee)

Updated

4 years ago
Depends on: 1008785
You need to log in before you can comment on or make changes to this bug.