Closed Bug 1329568 Opened 5 years ago Closed 5 years ago

New classes MediaMIMEType&friends to separate pure-data handling from context of MediaContentType

Categories

(Core :: Audio/Video: Playback, defect)

Product:
Core
Component:
Audio/Video: Playback
Version:
49 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

(Blocks 1 open bug)

Details

Attachments

(14 files)

Bug 1329568 - Add missing #include in MediaStreamListener.h -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1329568 - MediaExtendedMIMEType -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1329568 - MediaMIMEType -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1329568 - Simple IsMediaMIMEType checker for strings -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1329568 - Restrict MediaMIMEType to IsMediaMIMEType-checked strings -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1329568 - MediaMIMEType construction from literal string -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1329568 - MediaMIMEType comparisons against others -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1329568 - MediaMIMEType 'audio/' and 'video/' checks -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1329568 - MediaCodecs -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1329568 - Remove MediaContentType crutches -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1329568 - Media...Type::SizeOf... methods -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1329568 - Store original media-content string -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1329568 - gtest for MediaMIMEType and family -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1329568 - MediaSource::IsTypeSupported should report a NotSupportedError if type is not supported -
59 bytes, text/x-review-board-request
jya
: review+
Details 
MediaContentType does all the work of handling MIME type strings in the context of content types.

We want to separate the context (content types, what the data means to represent) from the data-handling (MIME strings, what the data actually is and how to manipulate its components in the safest way) for easier maintainability, and also because the same data-handling could potentially be used in different contexts.

To see the proposed patches in a broader context:
https://hg.mozilla.org/try/pushloghtml?changeset=2743aefd9eede9f207238ba71a48ccb5382af848
Jean-Yves:

As per the description, there are more bugs&patches to come after these, where I actually use the new types around dom/media, starting in DecoderTraits and its immediate dependencies, before going further.
Please have a look there if you'd like to see some example of these uses, that I hope will justify my design choices here:
https://hg.mozilla.org/try/pushloghtml?changeset=2743aefd9eede9f207238ba71a48ccb5382af848

But I will first let you review bug 1329561, bug 1329564, and this bug 1329568, before I throw more code at you!
Especially as your review comments here could lead to changes in my proposed later bugs&patches.
Comment on attachment 8824880 [details]
Bug 1329568 - Add missing #include in MediaStreamListener.h -

https://reviewboard.mozilla.org/r/103200/#review103834
Attachment #8824880 - Flags: review?(jyavenard) → review+
Comment on attachment 8824881 [details]
Bug 1329568 - MediaExtendedMIMEType -

https://reviewboard.mozilla.org/r/103202/#review103836
Attachment #8824881 - Flags: review?(jyavenard) → review+
Comment on attachment 8824882 [details]
Bug 1329568 - MediaMIMEType -

https://reviewboard.mozilla.org/r/103204/#review103838

::: dom/media/MediaContentType.h:22
(Diff revision 1)
>  class MediaContentType
>  {
>  public:
> +  explicit MediaContentType(const MediaMIMEType& aType)
> +    : mExtendedMIMEType(aType)
> +  {}

this is a bit inconsistent with the rest of the code that use:
{
}
on two lines

::: dom/media/MediaContentType.h:23
(Diff revision 1)
>  {
>  public:
> +  explicit MediaContentType(const MediaMIMEType& aType)
> +    : mExtendedMIMEType(aType)
> +  {}
> +  explicit MediaContentType(MediaMIMEType&& aType)

i don't think we need explicit for move constructor

::: dom/media/MediaMIMETypes.h:73
(Diff revision 1)
>    }
>  
> -  nsCString mMIMEType; // UTF8 MIME type.
> +  MediaMIMEType mMIMEType; // MIME type/subtype.
>    bool mHaveCodecs; // If false, mCodecs must be empty.
>    nsString mCodecs;
>    int32_t mWidth; // -1 if not provided.

please use C++11 initialization as mentioned below.

::: dom/media/MediaMIMETypes.cpp:85
(Diff revision 1)
>    return number;
>  }
>  
> +MediaExtendedMIMEType::MediaExtendedMIMEType(const MediaMIMEType& aType)
> +  : mMIMEType(aType)
> +  , mHaveCodecs(false)

C++11 initalization would be better
Attachment #8824882 - Flags: review?(jyavenard) → review+
Comment on attachment 8824883 [details]
Bug 1329568 - Simple IsMediaMIMEType checker for strings -

https://reviewboard.mozilla.org/r/103206/#review103840
Attachment #8824883 - Flags: review?(jyavenard) → review+
Comment on attachment 8824884 [details]
Bug 1329568 - Restrict MediaMIMEType to IsMediaMIMEType-checked strings -

https://reviewboard.mozilla.org/r/103208/#review103842

::: dom/media/MediaMIMETypes.cpp:26
(Diff revision 2)
>    if (!NS_SUCCEEDED(rv) || mime.IsEmpty()) {
>      return false;
>    }
>  
> -  mMIMEType = NS_ConvertUTF16toUTF8(mime);
> +  NS_ConvertUTF16toUTF8 mime8(mime);
> +  if (!IsMediaMIMEType(mime8.Data(), mime8.Length())) {

couldn't we have a function taking the UTF8 string directly ?
Attachment #8824884 - Flags: review?(jyavenard) → review+
Comment on attachment 8824885 [details]
Bug 1329568 - MediaMIMEType construction from literal string -

https://reviewboard.mozilla.org/r/103210/#review103844

nice
Attachment #8824885 - Flags: review?(jyavenard) → review+
Comment on attachment 8824886 [details]
Bug 1329568 - MediaMIMEType comparisons against others -

https://reviewboard.mozilla.org/r/103212/#review103846
Attachment #8824886 - Flags: review?(jyavenard) → review+
Comment on attachment 8824887 [details]
Bug 1329568 - MediaMIMEType 'audio/' and 'video/' checks -

https://reviewboard.mozilla.org/r/103214/#review103848

::: dom/media/MediaMIMETypes.h:79
(Diff revision 2)
>    bool operator!=(const MediaMIMEType& aOther) const
>    {
>      return !mMIMEType.Equals(aOther.mMIMEType);
>    }
>  
> +  bool IsAudio() const;

One has to be careful that video/blah is a valid mimetype for a container with only an audio track.

e.g.
video/webm
can contain a combination of one or more of the following:
video/vp9 video/vp8 audio/opus audio/vorbis
Attachment #8824887 - Flags: review?(jyavenard) → review+
Comment on attachment 8824888 [details]
Bug 1329568 - MediaCodecs -

https://reviewboard.mozilla.org/r/103216/#review103850

::: dom/media/MediaMIMETypes.cpp:177
(Diff revision 2)
>      return false;
>    }
>  
>    mMIMEType.SetType(nsCString(mime8));
>  
> -  rv = parser.GetParameter("codecs", mCodecs);
> +  nsAutoString codecs;

Unused variable?
Attachment #8824888 - Flags: review?(jyavenard) → review+
Comment on attachment 8824889 [details]
Bug 1329568 - Remove MediaContentType crutches -

https://reviewboard.mozilla.org/r/103218/#review103852
Attachment #8824889 - Flags: review?(jyavenard) → review+
Comment on attachment 8824890 [details]
Bug 1329568 - Media...Type::SizeOf... methods -

https://reviewboard.mozilla.org/r/103220/#review103854

LGTM, want to run that passed njn...
Attachment #8824890 - Flags: review?(jyavenard) → review+
Comment on attachment 8824891 [details]
Bug 1329568 - Store original media-content string -

https://reviewboard.mozilla.org/r/103222/#review103856

::: dom/media/MediaContentType.h:38
(Diff revision 2)
>    const MediaMIMEType& Type() const { return mExtendedMIMEType.Type(); }
>    const MediaExtendedMIMEType& ExtendedType() const { return mExtendedMIMEType; }
>  
> +  // Original string. Note that "type/subtype" may not be lowercase,
> +  // use Type().AsString() instead.
> +  const nsACString& AsString() const { return mExtendedMIMEType.AsString(); }

I think we should make the name a bit more explicit that it's the original string.

AsOriginalString maybe?
Attachment #8824891 - Flags: review?(jyavenard) → review+
Comment on attachment 8824892 [details]
Bug 1329568 - gtest for MediaMIMEType and family -

https://reviewboard.mozilla.org/r/103224/#review103858
Attachment #8824892 - Flags: review?(jyavenard) → review+
Attachment #8824890 - Flags: review?(n.nethercote)
Comment on attachment 8824890 [details]
Bug 1329568 - Media...Type::SizeOf... methods -

https://reviewboard.mozilla.org/r/103220/#review103990

Thank you for asking me to check this. Looks good except for one small thing.

::: dom/media/MediaContentType.cpp:16
(Diff revision 2)
>  namespace mozilla {
>  
> +size_t
> +MediaContentType::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const
> +{
> +  return mExtendedMIMEType.SizeOfIncludingThis(aMallocSizeOf);

This should call SizeOfExcludingThis() because mExtendedMIMEType is an inline member, not a pointer.

(This mistake can lead to crashes, because aMallocSizeOf will be called on an invalid pointer. Have you tested this code?)

(BTW, https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Memory_reporting is the docs for writing memory reporters, if you haven't seen it before.)
Attachment #8824890 - Flags: review?(n.nethercote) → review+
Blocks: 1330284
Comment on attachment 8824882 [details]
Bug 1329568 - MediaMIMEType -

https://reviewboard.mozilla.org/r/103204/#review103838

> this is a bit inconsistent with the rest of the code that use:
> {
> }
> on two lines

I guess I was trying to save vertical space in a header.
But I suppose consistency is best.
I'll fix these in other files, if any.

> i don't think we need explicit for move constructor

It's a converting constructor (the argument is of a different type from the class), so we definitely need an explicit here.

> please use C++11 initialization as mentioned below.

You could have told me in bug 1329561, or even in the previous patch! :-)
I intentionally used old-school initialization through constructors only, as there is an empty constructor that doesn't do anything followed by Populate() to actually initialize the values.
But I have found a way to instead use a fully-argumented constructor instead, which I introduce in the previous patch ("MediaExtendedMIMEType"). And in this patch here I introduce member in-line initialization, which is now needed as there are new constructors that don't need to give custum values to all members.
Comment on attachment 8824884 [details]
Bug 1329568 - Restrict MediaMIMEType to IsMediaMIMEType-checked strings -

https://reviewboard.mozilla.org/r/103208/#review103842

> couldn't we have a function taking the UTF8 string directly ?

Sure, easy enough.
Comment on attachment 8824887 [details]
Bug 1329568 - MediaMIMEType 'audio/' and 'video/' checks -

https://reviewboard.mozilla.org/r/103214/#review103848

> One has to be careful that video/blah is a valid mimetype for a container with only an audio track.
> 
> e.g.
> video/webm
> can contain a combination of one or more of the following:
> video/vp9 video/vp8 audio/opus audio/vorbis

These only test the major type, i.e., whether the string starts with "audio/" or "video/".
But I see that the name could give the wrong impression that they will definitely classify a type as audio or video, which could be incorrect as you demonstrate.
So I will rename them to "Has{Audio,Video}MajorType", and add comments.

While at it, I will also add HasApplicationMajorType, as it is an accepted type for some formats.
Comment on attachment 8824888 [details]
Bug 1329568 - MediaCodecs -

https://reviewboard.mozilla.org/r/103216/#review103850

> Unused variable?

Well-spotted!
In the work needed to add in-line member initializers, I've already fixed it -- it's still there, but it's actually used.
Comment on attachment 8824890 [details]
Bug 1329568 - Media...Type::SizeOf... methods -

https://reviewboard.mozilla.org/r/103220/#review103990

> This should call SizeOfExcludingThis() because mExtendedMIMEType is an inline member, not a pointer.
> 
> (This mistake can lead to crashes, because aMallocSizeOf will be called on an invalid pointer. Have you tested this code?)
> 
> (BTW, https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Memory_reporting is the docs for writing memory reporters, if you haven't seen it before.)

Thank you for finding this issue.
Also, as discussed on IRC, I will remove the unneeded 'Including' functions, as the size is only needed when these objects are directly included in others.
Comment on attachment 8824891 [details]
Bug 1329568 - Store original media-content string -

https://reviewboard.mozilla.org/r/103222/#review103856

> I think we should make the name a bit more explicit that it's the original string.
> 
> AsOriginalString maybe?

Good idea.
'OriginalString()' should do, as I think of it as a different beast -- giving an unmodified copy of some old data -- compared to e.g.: Type().AsString(), which gives us current data converted to a different type.
I had forgotten to publish my review responses, sorry.
I've already requested autoland, so if you see anything that's still a concern, I'll open a follow-up bug.
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1ed12f057c8
Add missing #include in MediaStreamListener.h - r=jya
https://hg.mozilla.org/integration/autoland/rev/644f206d06d6
MediaExtendedMIMEType - r=jya
https://hg.mozilla.org/integration/autoland/rev/2ce9dcf0c274
MediaMIMEType - r=jya
https://hg.mozilla.org/integration/autoland/rev/99758b5f7918
Simple IsMediaMIMEType checker for strings - r=jya
https://hg.mozilla.org/integration/autoland/rev/7c62078beb88
Restrict MediaMIMEType to IsMediaMIMEType-checked strings - r=jya
https://hg.mozilla.org/integration/autoland/rev/662529436cc4
MediaMIMEType construction from literal string - r=jya
https://hg.mozilla.org/integration/autoland/rev/453851b40e3e
MediaMIMEType comparisons against others - r=jya
https://hg.mozilla.org/integration/autoland/rev/3a49367220e3
MediaMIMEType 'audio/' and 'video/' checks - r=jya
https://hg.mozilla.org/integration/autoland/rev/da855d6a78bb
MediaCodecs - r=jya
https://hg.mozilla.org/integration/autoland/rev/90a091e65db3
Remove MediaContentType crutches - r=jya
https://hg.mozilla.org/integration/autoland/rev/c316c8d24d32
Media...Type::SizeOf... methods - r=jya,njn
https://hg.mozilla.org/integration/autoland/rev/706da9f85272
Store original media-content string - r=jya
https://hg.mozilla.org/integration/autoland/rev/8f37ee96bd78
gtest for MediaMIMEType and family - r=jya
The wpt failures came from bug 1176218, but were not apparent until now as I've added extra checks that can actually reject invalid&unexpected media types.
A new patch corrects this issue before the new checks are introduced.
Blocks: 1176218
Flags: needinfo?(gsquelart)
Comment on attachment 8826480 [details]
Bug 1329568 - MediaSource::IsTypeSupported should report a NotSupportedError if type is not supported -

https://reviewboard.mozilla.org/r/104442/#review105368

WHO R+ THAT ESRLIER?????
Attachment #8826480 - Flags: review?(jyavenard) → review+
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/47e7a628e4eb
Add missing #include in MediaStreamListener.h - r=jya
https://hg.mozilla.org/integration/autoland/rev/9a83e282245d
MediaExtendedMIMEType - r=jya
https://hg.mozilla.org/integration/autoland/rev/c38a9125908a
MediaMIMEType - r=jya
https://hg.mozilla.org/integration/autoland/rev/efa49d608777
MediaSource::IsTypeSupported should report a NotSupportedError if type is not supported - r=jya
https://hg.mozilla.org/integration/autoland/rev/170324f26108
Simple IsMediaMIMEType checker for strings - r=jya
https://hg.mozilla.org/integration/autoland/rev/09e268ea2617
Restrict MediaMIMEType to IsMediaMIMEType-checked strings - r=jya
https://hg.mozilla.org/integration/autoland/rev/ad28a64c910d
MediaMIMEType construction from literal string - r=jya
https://hg.mozilla.org/integration/autoland/rev/db6199820ea5
MediaMIMEType comparisons against others - r=jya
https://hg.mozilla.org/integration/autoland/rev/99bea6f5f7cb
MediaMIMEType 'audio/' and 'video/' checks - r=jya
https://hg.mozilla.org/integration/autoland/rev/24d6f93f708b
MediaCodecs - r=jya
https://hg.mozilla.org/integration/autoland/rev/2263f931b739
Remove MediaContentType crutches - r=jya
https://hg.mozilla.org/integration/autoland/rev/a5149be026c3
Media...Type::SizeOf... methods - r=jya,njn
https://hg.mozilla.org/integration/autoland/rev/c1bcd1c8c386
Store original media-content string - r=jya
https://hg.mozilla.org/integration/autoland/rev/dc460f55e042
gtest for MediaMIMEType and family - r=jya
for informational purposes only (no action needed), this set of patches showed a small increase (+1) in the number of constructors:
== Change summary for alert #4804 (as of January 13 2017 22:23 UTC) ==

Regressions:

  1%  compiler_metrics num_constructors linux32 debug     181 -> 182
  1%  compiler_metrics num_constructors linux64 debug     181 -> 182

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4804
You need to log in before you can comment on or make changes to this bug.