Cleanup DecoderTraits

RESOLVED FIXED in mozilla22

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

Trunk
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 9 obsolete attachments)

8.01 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
6.60 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
796 bytes, patch
tzimmermann
: review+
Details | Diff | Splinter Review
9.81 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
4.77 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
This is a followup for bug 826055.

The class DecoderTraits in content/media/DecoderTraits.{cpp,h} contains several functions for checking if certain types of files are supported. The exact types and functions depend on the platform and build flags. It should be investigated if the interface can be made platform-independent, and platform- and build-specific details can be moved to the implementation.
One idea is to add a function 'IsSupportedType', which tests all supported types; and implement the function 'nsContentUtils::FindInternalContentViewer' on top of it.
So what I'd been thinking is that instead of defining the various issupported functions only if the right #define is present, always defining them but just having them return false if the right #define is not present.
Well, we could do that to get some cleaner interfaces. But the only user besides nsHTMLMediaElement is nsContentUtils, which is system-depended and has to use these #defines as well.
Not arguing against your idea. Just saying it might be kind of pointless.
My point was that if we do it my way we could remove all the ifdefs from nsContentUtils, no?
Sorry, I mixed up the file names. I was talking about nsHTMLMediaElement::CreateDecoder at

  https://hg.mozilla.org/mozilla-central/file/2cc710018b14/content/html/content/src/nsHTMLMediaElement.cpp#l2162

This method creates a decoder for a mime type. The decoder is type-specific and does not exist if the respective #define isn't set. In the end, each of the Is<type>Type methods from DecoderTraits only gets called when its respective #define is set anyway.

If we really want to cleanup the #ifdefs, I propose to make CreateDecoder part of DecoderTraits. If you're OK with that idea, I'd supply a patch for it.
I don't really have a strong opinion about the CreateDecoder bit... But yes, if we can put all the ifdefs in DecoderTraits without losing anything in the process, that sounds great to me.
I've been thinking about this recently, but I never got around to refactoring it.

I think we should add:

* DecoderTraits::IsSupportedInVideoDocument(type) (for the nsContentUtils case) and,
* DecoderTraits::CreateDecoder(type) for the nsHTMLMediaElement::CreateDecoder case.

We don't want a base DecoderTraits::IsSupportedType function because some types that we do "support" (i.e. can play) we don't want to load in an nsVideoDocument (like WAV, RAW), which is what the logic in nsContentUtils is making the decision about.
Created attachment 719428 [details] [diff] [review]
Implement CreateDecoder in DecoderTraits
Attachment #719428 - Flags: review?(cpearce)
Created attachment 719429 [details] [diff] [review]
Implement IsSupportedTypeInVideo in DecoderTraits
Attachment #719429 - Flags: review?(cpearce)
Created attachment 719430 [details] [diff] [review]
Implement CreateReader in DecoderTraits
Attachment #719430 - Flags: review?(cpearce)
Created attachment 719431 [details] [diff] [review]
Don't export codec functions from DecoderTraits
Attachment #719431 - Flags: review?(cpearce)
Here are some patches for this issue. Sorry for the long delay. I got busy with other things.
Comment on attachment 719428 [details] [diff] [review]
Implement CreateDecoder in DecoderTraits

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

Great! Thanks for doing this. Just a few little nits.

::: content/html/content/public/nsHTMLMediaElement.h
@@ -379,5 @@
>     */
>    already_AddRefed<DOMMediaStream> CaptureStreamInternal(bool aFinishWhenEnded);
>  
>    /**
> -   * Create a decoder for the given aMIMEType. Returns null if we

Put this comment in DecoderTraits.h please.

::: content/media/DecoderTraits.cpp
@@ +386,5 @@
> +already_AddRefed<MediaDecoder>
> +DecoderTraits::CreateDecoder(const nsACString& aType, MediaDecoderOwner* aOwner)
> +{
> +  // If you change this list to add support for new decoders for codecs that
> +  // can be used by <audio>, please consider updating MediaDecodeTask::CreateDecoder

May as well remove this comment; we'll be creating readers in DecoderTraits once this lands anyway, so the comment will become out of date then.
Attachment #719428 - Flags: review?(cpearce) → review+
Attachment #719429 - Flags: review?(cpearce) → review+
Comment on attachment 719430 [details] [diff] [review]
Implement CreateReader in DecoderTraits

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

::: content/media/webaudio/MediaBufferDecoder.cpp
@@ +20,5 @@
>  #include "nsIScriptContext.h"
>  #include "nsIScriptObjectPrincipal.h"
>  #include "nsIScriptError.h"
>  #include "nsMimeTypes.h"
>  

I think the make file in this directory can now remove the "CFLAGS   += $(GSTREAMER_CFLAGS)" ? Can you test if we build without that, and if so include removing the GSTREAMER_CFLAGS in this patch please?

@@ +395,5 @@
>  
>    MOZ_ASSERT(!mBufferDecoder);
>    mBufferDecoder = new BufferDecoder(resource);
>  
>    // If you change this list to add support for new decoders, please consider

We don't need this comment here anymore.
Attachment #719430 - Flags: review?(cpearce) → review+
Comment on attachment 719431 [details] [diff] [review]
Don't export codec functions from DecoderTraits

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

Nice. Thanks!
Attachment #719431 - Flags: review?(cpearce) → review+
Created attachment 719857 [details] [diff] [review]
Implement CreateDecoder in DecoderTraits
Attachment #719428 - Attachment is obsolete: true
Attachment #719857 - Flags: review+
Created attachment 719858 [details] [diff] [review]
Implement IsSupportedTypeInVideo in DecoderTraits
Attachment #719429 - Attachment is obsolete: true
Attachment #719858 - Flags: review+
Created attachment 719860 [details] [diff] [review]
Implement CreateReader in DecoderTraits
Attachment #719430 - Attachment is obsolete: true
Attachment #719860 - Flags: review+
Created attachment 719862 [details] [diff] [review]
Don't export codec functions from DecoderTraits
Attachment #719431 - Attachment is obsolete: true
Attachment #719862 - Flags: review+
Created attachment 719863 [details] [diff] [review]
Remove GStreamer flags from webaudio Makefile

I updated the other patches according to your review. This patch contains the change to the makefile. I can't really test it, because I'm on b2g and there is no gstreamer here. Some feedback from Linux devs would be nice. Thanks!
Attachment #719863 - Flags: review?(cpearce)
Comment on attachment 719863 [details] [diff] [review]
Remove GStreamer flags from webaudio Makefile

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

I haven't tested this, but it should work.
Attachment #719863 - Flags: review?(cpearce) → review+
Created attachment 720578 [details] [diff] [review]
Remove GStreamer flags from webaudio Makefile
Assignee: nobody → tzimmermann
Attachment #719863 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #720578 - Flags: review+
Ok, thanks a lot.
Keywords: checkin-needed
Backed out for Android build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/38f04d4b03f9

https://tbpl.mozilla.org/php/getParsedLog.php?id=20298037&tree=Mozilla-Inbound

SVGMotionSMILAttr.cpp
/tools/android-ndk-r8c/toolchains/arm-linux-androideabi-4.6/prebuilt/linux-x86/bin/../lib/gcc/arm-linux-androideabi/4.6/../../../../arm-linux-androideabi/bin/as: /lib/libz.so.1: no version information available (required by /tools/android-ndk-r8c/toolchains/arm-linux-androideabi-4.6/prebuilt/linux-x86/bin/../lib/gcc/arm-linux-androideabi/4.6/../../../../arm-linux-androideabi/bin/as)
../../../content/media/DecoderTraits.cpp: In static member function 'static already_AddRefed<mozilla::MediaDecoder> mozilla::DecoderTraits::CreateDecoder(const nsACString_internal&, mozilla::MediaDecoderOwner*)':
../../../content/media/DecoderTraits.cpp:421:29: error: 'IsMediaPluginsEnabled' was not declared in this scope
make[6]: *** [DecoderTraits.o] Error 1
make[6]: Leaving directory `/builds/slave/m-in-and-a6-000000000000000000/build/obj-firefox/content/media'
make[5]: *** [media_libs] Error 2
Created attachment 720735 [details] [diff] [review]
Implement CreateDecoder in DecoderTraits

I don't know the official procedure if patch sets get backed out, so I'll just attach the fixed files for another review. This one adds the MediaDecoder namespace to the failed call.
Attachment #719857 - Attachment is obsolete: true
Attachment #720735 - Flags: review?(cpearce)
Created attachment 720736 [details] [diff] [review]
Implement IsSupportedTypeInVideo in DecoderTraits

And for completeness, this change removes the mozilla namespace from a call as we're already within mozilla.
Attachment #719858 - Attachment is obsolete: true
Attachment #720736 - Flags: review?(cpearce)
It's also a little confusing having two patches in the same series with identical commit messages.
Attachment #720735 - Flags: review?(cpearce) → review+
Attachment #720736 - Flags: review?(cpearce) → review+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #29)
> It's also a little confusing having two patches in the same series with
> identical commit messages.

Which ones? There is one patch for CreateReader and one for CreateDecoder. The other commit messages seem sufficiently different to me.
Created attachment 721098 [details] [diff] [review]
Implement CreateDecoder in DecoderTraits
Attachment #720735 - Attachment is obsolete: true
Attachment #721098 - Flags: review+
Created attachment 721099 [details] [diff] [review]
Implement IsSupportedTypeInVideo in DecoderTraits
Attachment #720736 - Attachment is obsolete: true
Attachment #721099 - Flags: review+
Keywords: checkin-needed
(In reply to Thomas Zimmermann [:tzimmermann] from comment #30)
> Which ones? There is one patch for CreateReader and one for CreateDecoder.
> The other commit messages seem sufficiently different to me.

Reading fail. Sorry about that.

Comment 35

5 years ago
Thanks a lot for doing this, Thomas!

Updated

5 years ago
Depends on: 848661
Depends on: 829653
Depends on: 849067
You need to log in before you can comment on or make changes to this bug.