Closed Bug 987064 Opened 6 years ago Closed 6 years ago

Default AudioChannel is wrong in HTMLMediaElements

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 6 obsolete files)

Attached patch constructor.patch (obsolete) — Splinter Review
No description provided.
Attachment #8395586 - Flags: review?(roc)
Comment on attachment 8395586 [details] [diff] [review]
constructor.patch

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

Currently we have duplicated information. WebIDL generates a mapping between strings and AudioChannelType. You're duplicating that mapping in kMozAudioChannelAttributeTable. Would it be possible to get rid of AudioChannelType completely and just use the AudioChannel enum? And use the WebIDL mapping for all mapping between strings and AudioChannel values?
Attachment #8395586 - Flags: review?(roc) → review-
Attached patch act.patch (obsolete) — Splinter Review
Would be nice if we can use AudioChannel in IDL files.
Attachment #8395586 - Attachment is obsolete: true
Attachment #8395633 - Flags: review?(roc)
Comment on attachment 8395633 [details] [diff] [review]
act.patch

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

Fantastic! This is a great cleanup.

::: dom/audiochannel/AudioChannelService.cpp
@@ +87,5 @@
>  NS_IMPL_ISUPPORTS2(AudioChannelService, nsIObserver, nsITimerCallback)
>  
>  AudioChannelService::AudioChannelService()
> +: mCurrentHigherChannel(1000)
> +, mCurrentVisibleHigherChannel(1000)

INT32_MAX?
Attachment #8395633 - Flags: review?(roc) → review+
Attached patch patch (obsolete) — Splinter Review
Attachment #8395633 - Attachment is obsolete: true
Attachment #8396328 - Flags: review?(mchen)
Attached patch id.patch (obsolete) — Splinter Review
The patch was not fully green on try. This is the interdiff. Roc can you spend 2 secs reviewing it?

The issues are:

1. MediaStreamGraphImpl::CreateOrDestroyAudio is called in a different process so Preferences obj asserts.

2. TabMessageUtils.h - the template works with int32_t but AudioChannel is an enum of uint32_t so it doesn't compile. I had to implement the struct ParamTraits<mozilla::dom::AudioChannel>.
Attachment #8396330 - Flags: review?(roc)
Comment on attachment 8396328 [details] [diff] [review]
patch

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

Hi Andrea,

Generally it looks good to me and it's great.
Only a few points need your clarify. Thanks.

::: content/base/src/nsAttrValue.h
@@ +11,5 @@
>  #ifndef nsAttrValue_h___
>  #define nsAttrValue_h___
>  
>  #include "nscore.h"
> +#include "nsStringGlue.h"

Is the related to this patch?

::: content/svg/content/src/SVGAttrValueWrapper.h
@@ +11,5 @@
>   * Utility wrapper for handling SVG types used inside nsAttrValue so that these
>   * types don't need to be exported outside the SVG module.
>   */
>  
> +#include "nsStringGlue.h"

Is the change related to this bug?

::: dom/audiochannel/AudioChannelCommon.h
@@ -12,5 @@
>  
> -// The audio channel. Read the nsIHTMLMediaElement.idl for a description
> -// about this attribute.
> -enum AudioChannelType {
> -  AUDIO_CHANNEL_DEFAULT = -1,

It seems that you didn't modify the AudioChannelManager.cpp regarding to this.
http://hg.mozilla.org/mozilla-central/file/441f5fd256e2/dom/system/gonk/AudioChannelManager.cpp#l28
http://hg.mozilla.org/mozilla-central/file/441f5fd256e2/dom/system/gonk/AudioChannelManager.cpp#l156

::: dom/ipc/PContent.ipdl
@@ +520,5 @@
>                                      bool aElementHidden,
>                                      bool aWithVideo);
>  
>      async AudioChannelChangedNotification();
> +    async AudioChannelChangeDefVolChannel(int32_t aChannel, bool aHidden);

May I now that why don't we use AudioChannel instead of int32_t like others?
And we don't need to do static_cast on AudioChannelService.

I withdraw my comment because you need to specify a "-1" for default channel.

::: dom/ipc/TabMessageUtils.h
@@ +64,5 @@
> +{
> +  typedef mozilla::dom::AudioChannel paramType;
> +
> +  static bool IsLegalValue(const paramType &aValue) {
> +    return aValue <= mozilla::dom::AudioChannel::Publicnotification;

It seems that we can check the minimal value too.
Attachment #8396328 - Flags: review?(mchen)
Hi Andrea,

By the way, could you ask IPDL part to another reviewer?
I have no confident for these codes. Thanks.
> >  #include "nscore.h"
> > +#include "nsStringGlue.h"
> 
> Is the related to this patch?

Yes. Otherwise we cannot compile the TestAudioChannelService.cpp.
String.h cannot be used in CPP unit tests. There StringGlue.h for that.

> It seems that you didn't modify the AudioChannelManager.cpp regarding to
> this.
> http://hg.mozilla.org/mozilla-central/file/441f5fd256e2/dom/system/gonk/
> AudioChannelManager.cpp#l28
> http://hg.mozilla.org/mozilla-central/file/441f5fd256e2/dom/system/gonk/
> AudioChannelManager.cpp#l156

Correct. I have an update for that. 

> May I now that why don't we use AudioChannel instead of int32_t like others?
> And we don't need to do static_cast on AudioChannelService.

Because there is 'default' that is -1. It's a your patch that introduces this value :)
But we don't want to expose 'default' as AudioChannel.

> ::: dom/ipc/TabMessageUtils.h
> @@ +64,5 @@
> > +{
> > +  typedef mozilla::dom::AudioChannel paramType;
> > +
> > +  static bool IsLegalValue(const paramType &aValue) {
> > +    return aValue <= mozilla::dom::AudioChannel::Publicnotification;
> 
> It seems that we can check the minimal value too.

AudioChannel enum is uint32_t so it cannot be < 0 so we don't have to check the minimal value.
Attached patch id.patch (obsolete) — Splinter Review
This is the interdiff for the AudioChannelManager.cpp and other stuff.
Attachment #8396330 - Attachment is obsolete: true
Attachment #8397746 - Flags: review?(mchen)
Attached patch patch (obsolete) — Splinter Review
bent, can you review the IPDL part? Thanks.
Attachment #8396328 - Attachment is obsolete: true
Attachment #8397747 - Flags: review?(bent.mozilla)
Comment on attachment 8397746 [details] [diff] [review]
id.patch

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

It looks good to me.
r+ after addressing the only one question. 

Thanks.

::: dom/audiochannel/tests/moz.build
@@ +8,5 @@
>      'TestAudioChannelService.cpp',
>  ]
>  
> +if CONFIG['OS_ARCH'] == 'WINNT':
> +    DEFINES['NOMINMAX'] = True

May I know which modification caused this change?
It seems to be related to "double definition of the min and max macros" but I don't remember the relevant codes.
Attachment #8397746 - Flags: review?(mchen) → review+
> May I know which modification caused this change?
> It seems to be related to "double definition of the min and max macros" but
> I don't remember the relevant codes.

windows compiler complains about std::min std::max and so on if that macro is not set. I didn't investigate too much but in many other places of gecko source code there is the use of this macro for the same reason.
Yes, Windows headers are silly!
Got it. Thanks.
Comment on attachment 8397747 [details] [diff] [review]
patch

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

::: dom/ipc/TabMessageUtils.h
@@ -59,5 @@
>  };
>  
> -template <>
> -struct ParamTraits<mozilla::dom::AudioChannelType>
> -  : public EnumSerializer<mozilla::dom::AudioChannelType,

Er, shouldn't this just inherit TypedEnumSerializer?
> > -struct ParamTraits<mozilla::dom::AudioChannelType>
> > -  : public EnumSerializer<mozilla::dom::AudioChannelType,
> 
> Er, shouldn't this just inherit TypedEnumSerializer?

No. TypedEnumSerializer does this:

    return smallestLegal <= intParamType(aValue) && intParamType(aValue) < highBound;

but AudioChannel is a uint32_t enum so AudioChannelNormal is 0 and nothing can be < 0 if uint32_t.
This is produces a compilation error. This is the reason why we have this custom template.
Comment on attachment 8397747 [details] [diff] [review]
patch

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

r=me on the dom/ipc changes
Attachment #8397747 - Flags: review?(bent.mozilla) → review+
(In reply to Andrea Marchesini (:baku) from comment #17)
> > > -struct ParamTraits<mozilla::dom::AudioChannelType>
> > > -  : public EnumSerializer<mozilla::dom::AudioChannelType,
> > 
> > Er, shouldn't this just inherit TypedEnumSerializer?
> 
> No. TypedEnumSerializer does this:
> 
>     return smallestLegal <= intParamType(aValue) && intParamType(aValue) <
> highBound;
> 
> but AudioChannel is a uint32_t enum so AudioChannelNormal is 0 and nothing
> can be < 0 if uint32_t.
> This is produces a compilation error. This is the reason why we have this
> custom template.

Would you file a bug on this so TypedEnumSerializer can do the right thing here?
> Would you file a bug on this so TypedEnumSerializer can do the right thing
> here?

I don't think we have a need for this at the moment. TypedEnumSerializer works fine with 'standard' enum. Maybe we can implement  TypedUnsignedEnumSerializer. How does it sound?
(In reply to Andrea Marchesini (:baku) from comment #20)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ac06eacc2206

Hi Baku, sorry had to back this out for build failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=37561242&tree=Mozilla-Inbound
(In reply to Andrea Marchesini (:baku) from comment #21)
> > Would you file a bug on this so TypedEnumSerializer can do the right thing
> > here?
> 
> I don't think we have a need for this at the moment. TypedEnumSerializer
> works fine with 'standard' enum. Maybe we can implement 
> TypedUnsignedEnumSerializer. How does it sound?

I think we do have a need for it because you had to work around this issue. :)

No need to have something separate; TypedEnumSerializer can figure out that the type is unsigned on its own and perform the correct checks if the lower bound is zero.
Attached patch patchSplinter Review
Attachment #8397746 - Attachment is obsolete: true
Attachment #8397747 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f31e8f1f24b9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.