834172 - Cleanup DecoderTraits

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

One idea is to add a function 'IsSupportedType', which tests all supported types; and implement the function 'nsContentUtils::FindInternalContentViewer' on top of it.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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.

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Not arguing against your idea. Just saying it might be kind of pointless.

Comment 5

[Boris Zbarsky [:bzbarsky]]

My point was that if we do it my way we could remove all the ifdefs from nsContentUtils, no?

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

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

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.

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Implement CreateDecoder in DecoderTraits

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Implement IsSupportedTypeInVideo in DecoderTraits

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Implement CreateReader in DecoderTraits

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Don't export codec functions from DecoderTraits

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Here are some patches for this issue. Sorry for the long delay. I got busy with other things.

Comment 14

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

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.

Comment 15

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

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.

Comment 16

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

Comment on attachment 719431 [details] [diff] [review]
Don't export codec functions from DecoderTraits

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

Nice. Thanks!

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Implement CreateDecoder in DecoderTraits

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Implement IsSupportedTypeInVideo in DecoderTraits

Comment 19

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Implement CreateReader in DecoderTraits

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Don't export codec functions from DecoderTraits

Comment 21

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: 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!

Comment 22

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

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.

Comment 23

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Remove GStreamer flags from webaudio Makefile

Comment 24

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Ok, thanks a lot.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f0cdf0cbb60
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4da9781b260
https://hg.mozilla.org/integration/mozilla-inbound/rev/6beb3290879a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f06028922aaf
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef997e94ecbc

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: 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.

Comment 28

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Implement IsSupportedTypeInVideo in DecoderTraits

And for completeness, this change removes the mozilla namespace from a call as we're already within mozilla.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

It's also a little confusing having two patches in the same series with identical commit messages.

Comment 30

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(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.

Comment 31

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Implement CreateDecoder in DecoderTraits

Comment 32

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Implement IsSupportedTypeInVideo in DecoderTraits

Comment 33

[Ryan VanderMeulen [:RyanVM]]

(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 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd75eab24983
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd63d49eb5dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/128510dd6c03
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d3ed0bd266
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6594d068e74

Comment 35

[(no longer active)]

Thanks a lot for doing this, Thomas!

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dd75eab24983
https://hg.mozilla.org/mozilla-central/rev/dd63d49eb5dc
https://hg.mozilla.org/mozilla-central/rev/128510dd6c03
https://hg.mozilla.org/mozilla-central/rev/e2d3ed0bd266
https://hg.mozilla.org/mozilla-central/rev/b6594d068e74