Environment: Aurora (50.0a2) Device: Samsung Galaxy S6 EDGE (Android 6.0 ); Build: Aurora 50.0a2 (2016-09-07); Steps to reproduce: 1. Go to goo.gl/LLxEY8 and play the HLS video 2. Wait for the video to load and exit the video player (back device button) 3. Reload the page Expected result: After step 2 : the video should be available to be played again Actual result: After step 2 : the error mentioned in bug 1301053 is still displayed and user cannot replay the video After step 3 : The video can be played

Anthony we talked about this a little bit and you had suggested someone who could help, but I don't remember who.

JW - can you help with this?

Can't repro the issue. Reloading the page will also reset the media element to its pristine state so it can play again. Can you record a video to show what the issue is like?

Hi Wang, Here you can find the video with the above mentioned issue https://www.youtube.com/watch?v=Y56vlVQ9K4Y&feature=youtu.be

It sounds like a communication issue between the reporter and JW. The problem is that videos can only be played once. The workaround for the problem is to reload the page.

Thanks for clarifying the issue.

It looks like we should clear the error if the URI can be handled by an external app. Hi Alastor, since the HLS change is made by you, can you figure out a way to allow re-play of an HLS video? THanks.

The problem here is because we didn't notify video player again, if this media element had been played before [1]. The easy solution is to remove this return checking. However, if we want to have better UX for playing these kinds of unsupported media, we should change the video control interface to make user more clearly know they can replay it by clicking again. Now we could only see one error image "Video format or MIME type is not supported". In addition, I observed the clicking functionality is not very stable for video control. Sometime I can replay the unsupported media by double clicking, but sometime can't. It might be the issue for video control. [1] http://searchfox.org/mozilla-central/rev/950e13cca1fda6507dc53c243295044e8eda4493/mobile/android/chrome/content/browser.js#4511

Comment on attachment 8790186 [details] Bug 1301055 - part1 : allow to replay the same video again. https://reviewboard.mozilla.org/r/78118/#review76990 ::: mobile/android/chrome/content/browser.js:4512 (Diff revision 1) > - if (aEvent.target.moz_video_uuid) { > - return; > - } > let mediaSrc = aEvent.target.currentSrc || aEvent.target.src; > if (!aEvent.target.moz_video_uuid) { > aEvent.target.moz_video_uuid = uuidgen.generateUUID().toString(); We should stop setting this too

Comment on attachment 8790186 [details] Bug 1301055 - part1 : allow to replay the same video again. https://reviewboard.mozilla.org/r/78118/#review76992 One issue, otherwise ok.

Here I want to emphasize that we still can't replay unsupported media in any websites even with my patch, we can only replay it on our video control (if our video control also did some changing). That is because my patch allows media element notify Fennec using the android view to play the unsupported media if "xxx.play()" was called. Notice that, we can only replay the video if the media element's PLAY() was called by HTML5 video player. However, for this kinds of media, the media element would return the DOM error (MEDIA_ERR_SRC_NOT_SUPPORTED) that means the JS video player might not show any play button or allows users to replay this media element again. --- The conclusion is - For our native video player (videocontrols.xml), we can replay unsupported media if the video control can trigger the xxx.play() after receiving the DOM error (MEDIA_ERR_SRC_NOT_SUPPORTED) - For other custom JS video players, we can't guarantee whether they can replay unsupported media.

After discussion with Blake, another possible solution is we don't return DOM error when we starts playing the unsupported media, but returns "onended" event, and this solution would only be for Fennec. The idea is that, - If we don't throw DOM error, that means the JS video player won't show the error message so that users could start playing media again. - If we dispatch "onended" event when we starts to play unsupported media, the JS video player would change back to the pause state (showing the play button, instead of showing pause button) -- Hi, JW, Snorp, How do you guys think about above solution? Thanks!

Yup, I was thinking about something like that too. You can just see if someone called stopPropagation() on the "OpenMediaWithExternalApp" event to determine whether or not you should set the error event. We could avoid Fennec-specific ifdefs that way.

I don't like to insert something that doesn't belong to the spec because it makes it hard to align our implementation with spec changes. Since the error message is displayed by the JS video player, is it possible to change the behaviour of the player such that it doesn't show the error if it decides to open the link with an external app?

I agree this idea is not good enough, and it's little hack. The best solution is to implement HLS decoder, but I don't sure whether we have any plan about that. The problem is that user can customize JS video player, so we can't control the behavior after they receive the DOM error.

The proposed solution on comment 14 could also fix Bug 1301053.

JW, If comment 14 doesn't sound good to you, any other suggestions?

We can have a policy object to move all our custom/policy (ex: block-media, audio-channel...) code there. The policy object can choose to consume the error and open the URI with an external app or just bubble up the error and doesn't handle it.

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #20) > We can have a policy object to move all our custom/policy (ex: block-media, > audio-channel...) code there. The policy object can choose to consume the > error and open the URI with an external app or just bubble up the error and > doesn't handle it. This is a really great idea. IIUC, this is about the design in Gecko. How about the problem on the players on the website? How can they know this is not an error and user can play it again? Alastor and JW, could you discuss how to move this bug forward?

Sure, I'll start to work on that after finishing some media control bugs.

Hi guys, SoftVision has deemed this as a P1 issue with their "HLS Video" testing in Beta50. Are we considering this a high priority and planning to fix before we gtb 50.0b10 (Monday Oct 24th)? If not, do we plan to disable this feature from Fennec 50 altogether?

We've been talking about this on r-d and elsewhere, and I think the determination that a slightly-busted HLS player is better than nothing at all.

Agree with comment 24.

Agree with snorp.

SGTM.

Comment on attachment 8807012 [details] Bug 1301055 - part2 : create a error sink to handle media element's error related event. https://reviewboard.mozilla.org/r/90268/#review91068 Looks fine to me except the cycle collection parts. Please have a DOM peer to take a look.

Hi, Baku, Could you help me review the cycle collection parts? Thanks!

Comment on attachment 8808941 [details] Bug 1301055 - part3 : modify test prefs. Cancel r?, finding a way to change the pref for the crash test.

Comment on attachment 8808941 [details] Bug 1301055 - part3 : modify test prefs. https://reviewboard.mozilla.org/r/91654/#review91836 Isn't it only one of the pref files will take effect for your crash test? So you should only modify one of them, right?

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #43) > Isn't it only one of the pref files will take effect for your crash test? So > you should only modify one of them, right? Not only affect crashtest, but also other tests. This pref would be on in mobile environment, but we don't use external app to open the unsupported type media in testing environment. If we don't turn it off, it also caused some other mochitest fails.

So basically you want to turn off external app for all kinds of tests, right?

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #45) > So basically you want to turn off external app for all kinds of tests, right? Yes.

Comment on attachment 8808941 [details] Bug 1301055 - part3 : modify test prefs. https://reviewboard.mozilla.org/r/91654/#review91844 r+ for amending the commit message to explain more details why turning off the pref.

Comment on attachment 8807012 [details] Bug 1301055 - part2 : create a error sink to handle media element's error related event. https://reviewboard.mozilla.org/r/90268/#review91946 ::: dom/html/HTMLMediaElement.h:767 (Diff revision 5) > class MediaStreamTracksAvailableCallback; > class MediaStreamTrackListener; > class StreamListener; > class StreamSizeListener; > class ShutdownObserver; > + class ErrorSink; alphabetic order. ::: dom/html/HTMLMediaElement.cpp:693 (Diff revision 5) > nsCOMPtr<nsIChannel> mChannel; > > bool mCancelled = false; > }; > > +class HTMLMediaElement::ErrorSink final : public nsISupports { { in a new line ::: dom/html/HTMLMediaElement.cpp:700 (Diff revision 5) > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_CLASS(ErrorSink) > + > + explicit ErrorSink(HTMLMediaElement* aOwner) > + : mOwner(aOwner) > + , mError(nullptr) remove this ::: dom/html/HTMLMediaElement.cpp:710 (Diff revision 5) > + return mError; > + } > + > + void SetError(uint16_t aErrorCode, const nsACString& aErrorDetails) > + { > + if (GetError()) { if (!mError) ::: dom/html/HTMLMediaElement.cpp:715 (Diff revision 5) > + if (GetError()) { > + return; > + } > + > + if (!IsValidErrorCode(aErrorCode)) { > + NS_ASSERTION(false, "Undefined MediaError codes!"); MOZ_CRASH() ? ::: dom/html/HTMLMediaElement.cpp:719 (Diff revision 5) > + if (!IsValidErrorCode(aErrorCode)) { > + NS_ASSERTION(false, "Undefined MediaError codes!"); > + return; > + } > + > + MOZ_ASSERT(mOwner); move this to the CTOR ::: dom/html/HTMLMediaElement.cpp:722 (Diff revision 5) > + } > + > + MOZ_ASSERT(mOwner); > + mError = new MediaError(mOwner, aErrorCode, aErrorDetails); > + > + if (CanPlayUnsupportedTypeMedia()) { This should no be part of ErrorSink. It's not the errorSink able to play unsupportedTypeMedia. Move to mOwner. ::: dom/html/HTMLMediaElement.cpp:724 (Diff revision 5) > + MOZ_ASSERT(mOwner); > + mError = new MediaError(mOwner, aErrorCode, aErrorDetails); > + > + if (CanPlayUnsupportedTypeMedia()) { > + mOwner->ChangeNetworkState(nsIDOMHTMLMediaElement::NETWORK_NO_SOURCE); > + mOwner->OpenUnsupportedMediaWithExtenalAppIfNeeded(); why we don't dispatch error? In the previous code we were doing it. I don't think you want to change the behavior of HTMLMediaElement in this bug. If yes, do it in a separate patch. ::: dom/html/HTMLMediaElement.cpp:725 (Diff revision 5) > + mError = new MediaError(mOwner, aErrorCode, aErrorDetails); > + > + if (CanPlayUnsupportedTypeMedia()) { > + mOwner->ChangeNetworkState(nsIDOMHTMLMediaElement::NETWORK_NO_SOURCE); > + mOwner->OpenUnsupportedMediaWithExtenalAppIfNeeded(); > + } else { If you follow my previous comment, you should do: switch (aErrorCode) { case MEDIA_ERR_ABORTED: ... break; case MEDIA_ERR_SRC_NOT_SUPPORTED: ... break; etc... } ::: dom/html/HTMLMediaElement.cpp:752 (Diff revision 5) > + > + bool IsValidErrorCode(const uint16_t& aErrorCode) const > + { > + return (aErrorCode == MEDIA_ERR_DECODE || > + aErrorCode == MEDIA_ERR_NETWORK || > + aErrorCode == MEDIA_ERR_ABORTED || indentation ::: dom/html/HTMLMediaElement.cpp:769 (Diff revision 5) > + } > +#endif > + return false; > + } > + > + HTMLMediaElement* mOwner; Write a comment about this raw pointer. ::: dom/html/HTMLMediaElement.cpp (Diff revision 5) > - } > - mError = new MediaError(this, aErrorCode, aErrorDetails); > - > - DispatchAsyncEvent(NS_LITERAL_STRING("error")); > - if (mReadyState == HAVE_NOTHING && aErrorCode == MEDIA_ERR_ABORTED) { > - // https://html.spec.whatwg.org/multipage/embedded-content.html#media-data-processing-steps-list check if this comment is useful and in case, don't remove it. ::: dom/html/HTMLMediaElement.cpp:5712 (Diff revision 5) > } > > +MediaError* > +HTMLMediaElement::GetError() const > +{ > + if (!mErrorSink) { how can it be that mErrorSink is null? ::: dom/html/HTMLMediaElement.cpp:5716 (Diff revision 5) > +{ > + if (!mErrorSink) { > + return nullptr; > + } > + > +return mErrorSink->GetError(); indentation ::: dom/html/HTMLMediaElement.cpp:5723 (Diff revision 5) > + > +void > +HTMLMediaElement::SetError(uint16_t aErrorCode, > + const nsACString& aErrorDetails) > +{ > + if (!mErrorSink) { same here. ::: dom/html/HTMLMediaElement.cpp:5733 (Diff revision 5) > +} > + > +void > +HTMLMediaElement::ResetError() > +{ > + if (!mErrorSink) { or here.

Comment on attachment 8807012 [details] Bug 1301055 - part2 : create a error sink to handle media element's error related event. https://reviewboard.mozilla.org/r/90268/#review92200

(In reply to Andrea Marchesini [:baku] from comment #49) > Comment on attachment 8807012 [details] > Bug 1301055 - part2 : create a error sink to handle media element's error > related event. > > https://reviewboard.mozilla.org/r/90268/#review91946 hmm...I don't know why my reply didn't be posted to bugzilla synchronously, but I have posted them on the review board.

Comment on attachment 8810020 [details] Bug 1301055 - part4 : special error dispatching stragedy for Fennec. https://reviewboard.mozilla.org/r/92238/#review92578 ::: dom/html/HTMLMediaElement.h:1284 (Diff revision 1) > bool IsTabActivated() const; > > bool IsAudible() const; > - bool HaveFailedWithSourceNotSupportedError() const; > > - void OpenUnsupportedMediaWithExtenalAppIfNeeded(); > + // We would user the external app to open unsupported type media if the error s/would user/will use/ s/the external/an external/ s/unsupported type media/unsupported media types/ Please move this comment into ErrorSink because it is about the policy. The comment should be as simple as "Open the media with an external app". ::: dom/html/HTMLMediaElement.cpp:3204 (Diff revision 1) > UpdateSrcMediaStreamPlaying(); > UpdateAudioChannelPlayingState(); > > // The check here is to handle the case that the media element starts playing > // after it loaded fail. eg. preload the data before playing. > - OpenUnsupportedMediaWithExtenalAppIfNeeded(); > + if (mErrorSink && mErrorSink->CanOwnerPlayUnsupportedTypeMedia()) { We want to move all the policy code about external app into ErrorSink and this statement breaks the encapsulation. We should let the ErrorSink handle this play request.

Comment on attachment 8810020 [details] Bug 1301055 - part4 : special error dispatching stragedy for Fennec. https://reviewboard.mozilla.org/r/92238/#review92926

review ping?

Hi, Ben, Could you help me review the cycle collection part? Thanks!

FYI, its not necessary to request needinfo on top of a review flag.

Comment on attachment 8807012 [details] Bug 1301055 - part2 : create a error sink to handle media element's error related event. https://reviewboard.mozilla.org/r/90268/#review95000 This would work, but I think its a bit dangerous as currently written. In particular, it doesn't seem like ErrorSink should really be ref-counted at all. Or, if it is ref-counted then the guarantee about its lifespan compared to HTMLMediaElement seems difficult to guarantee. ::: dom/html/HTMLMediaElement.cpp:697 (Diff revision 8) > > +class HTMLMediaElement::ErrorSink final : public nsISupports > +{ > +public: > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_CLASS(ErrorSink) So, below I recommend you don't make this class cycle collection at all. But for future reference: Are you only extending nsISupports in order to have ref-counting and cycle collection? If so, please don't. You can instead just have a non-isupports class and use this in your class declaration: ``` NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS(ErrorSink) NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(ErrorSink) ``` And then in the definition do: ``` NS_IMPL_CYCLE_COLLECTION_CLASS(HTMLMediaElement::ErrorSink) NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(HTMLMediaElement::ErrorSink) NS_IMPL_CYCLE_COLLECTION_UNLINK(mError) NS_IMPL_CYCLE_COLLECTION_UNLINK_END NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(HTMLMediaElement::ErrorSInk) NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mError) NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(HTMLMediaElement::ErrorSink, AddRef) NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(HTMLMediaElement::ErrorSink, Release) ``` ::: dom/html/HTMLMediaElement.cpp:758 (Diff revision 8) > + aErrorCode == MEDIA_ERR_SRC_NOT_SUPPORTED); > + } > + > + // Media elememt's life cycle would be longer than error sink, so we use the > + // raw pointer. > + HTMLMediaElement* mOwner; This is only safe if you can guarantee nothing else ever refs the ErrorSink. If something else can ref it then it might out live the HTMLMediaElement. If you can guarantee that, though, you might as well not make ErrorSink refcounting at all and instead have a UniquePtr<> in HTMLMediaElement. If you do that then you would need to do the following in your HTMLMediaElement code: ``` NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(HTMLMediaElement) NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mErrorSink->mError) NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(HTMLMediaElement) tmp->mErrorSink.reset(); NS_IMPL_CYCLE_COLLECTION_UNLINK_END ```

(In reply to Ben Kelly [:bkelly] from comment #69) > So, below I recommend you don't make this class cycle collection at all. Thanks, I'll try to remove the cycle collection for the mErrorSink and only do traverse for mErrorSink->mError. > This is only safe if you can guarantee nothing else ever refs the ErrorSink. > If something else can ref it then it might out live the HTMLMediaElement. > > If you can guarantee that, though, you might as well not make ErrorSink > refcounting at all and instead have a UniquePtr<> in HTMLMediaElement. If > you do that then you would need to do the following in your HTMLMediaElement > code: I want that the ErrorSink can only be referenced by HTMLMediaElement, I'll use the UniquePtr for that.

Comment on attachment 8807012 [details] Bug 1301055 - part2 : create a error sink to handle media element's error related event. https://reviewboard.mozilla.org/r/90268/#review95214 Thanks!

Do you think we should uplift this to 51?

Here I want to shortly describe what problem we got now and what would it goes after this bug. --- [How we play the unsupported types media] For unsupported types media, gecko doesn't have ability to play these files, so we use Android native method (VideoView) in bug1286133. Gecko would notify the java side and then trigger the VideoView. [Present problem] When play the unsupported types media, it can't be played twice. It has several reasons, (1) our implementation only notifies java side once - easy to correct, gecko can notify every-time users call video.play() (2) the JS video player unlocks its play button because of receiving the "error" event from media element - for Fennec, we use a workaround that is not to dispatch "error" event in this situation [Future problem] tl;dr - after stop the video, the JS player UI still shows the pause button instead of play button Since the VideoView is totally independent with the media element, their internal state doesn't be correspond well. For example, when user stop VideoView, media element wouldn't dispatch "pause" event. That causes the JS player's UI inconsistent with the actual playing state. When we stop the video from VideoView, the JS player doesn't know about that, so it would still show the pause button instead of play button. Users need to click the button twice(one is to recover button back to play, second time is to trigger play) to play the video again.

(In reply to Marco Castelluccio [:marco] from comment #76) > Do you think we should uplift this to 51? It seems too risky to uplift it to 51 (because we still need some time to test it), maybe we can uplift it to 52. --- Hi, Snorp, How do you think?

Pushed by alwu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a66b7c936f76 part1 : allow to replay the same video again. r=snorp https://hg.mozilla.org/integration/autoland/rev/7794b64cae28 part2 : create a error sink to handle media element's error related event. r=bkelly,jwwang https://hg.mozilla.org/integration/autoland/rev/cb8ff0bdb0f8 part3 : modify test prefs. r=jwwang https://hg.mozilla.org/integration/autoland/rev/165dd7c3d972 part4 : special error dispatching stragedy for Fennec. r=jwwang

https://hg.mozilla.org/mozilla-central/rev/a66b7c936f76 https://hg.mozilla.org/mozilla-central/rev/7794b64cae28 https://hg.mozilla.org/mozilla-central/rev/cb8ff0bdb0f8 https://hg.mozilla.org/mozilla-central/rev/165dd7c3d972

Verified as fixed on the latest Nightly build 53.0a1 (2016-11-25), tested on a Samsung Galaxy S6 EDGE (Android 6.0.1)

(In reply to Alastor Wu [:alwu] from comment #77) > [Future problem] > tl;dr - after stop the video, the JS player UI still shows the pause button > instead of play button > > Since the VideoView is totally independent with the media element, their > internal state doesn't be correspond well. For example, when user stop > VideoView, media element wouldn't dispatch "pause" event. Is it possible to detect when users stop the VideoView and dispatch the "pause" event when they do?

With the website from bug 1301053, a similar bug to this one is reproducible. While you're playing the video in the VideoView, the underlying media element is still trying to load the video (you can see the spinner spinning underneath the VideoView). Once you close the VideoView, you are presented with an error. I'm attaching a screenshot on bug 1301053.

(In reply to Alastor Wu [:alwu] from comment #78) > (In reply to Marco Castelluccio [:marco] from comment #76) > > Do you think we should uplift this to 51? > > It seems too risky to uplift it to 51 (because we still need some time to > test it), maybe we can uplift it to 52. > > --- > > Hi, Snorp, > How do you think? Yup, sounds fine.

I'll PTO tomorrow, set the flag for reminder.

Hi, JW, Could you help me review this patch? Because there is a merge error in "prefs_genral.js" when I tried to apply the present patches to aurora, so I created a new patch for that. Thanks!

Comment on attachment 8815559 [details] [diff] [review] bug1301055 (for aurora) sorry, wrong patch.

(In reply to Marco Castelluccio [:marco] from comment #87) > Is it possible to detect when users stop the VideoView and dispatch the > "pause" event when they do? I don't know the implementation details about the VideoView, but I guess that might be possible. We can file another bug to handle that issue.

(In reply to Alastor Wu [:alwu] from comment #91) > Created attachment 8815559 [details] [diff] [review] > bug1301055 (for aurora) > > Hi, JW, > Could you help me review this patch? > Because there is a merge error in "prefs_genral.js" when I tried to apply > the present patches to aurora, so I created a new patch for that. > Thanks! If prefs_genral.js is the only conflict, you don't need my review again. Just resolve the conflict and carry r+.

Comment on attachment 8815562 [details] [diff] [review] bug1301055 - allow HLS video cannot be played twice without reloading the whole page (for aurora) According to the comment95, carry the previous review flag. --- Approval Request Comment [Feature/Bug causing the regression]: Allow to replay the unsupported types media [User impact if declined]: They need to reload the whole page if they want to replay the media [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: No [Is the change risky?]: Almost no risk [Why is the change risky/not risky?]: This change is only for Fennec, and what it does is not to dispatch the "error" event when using the external app to play media [String changes made/needed]: No

(In reply to Alastor Wu [:alwu] from comment #94) > (In reply to Marco Castelluccio [:marco] from comment #87) > > Is it possible to detect when users stop the VideoView and dispatch the > > "pause" event when they do? > > I don't know the implementation details about the VideoView, but I guess > that might be possible. > We can file another bug to handle that issue. I filed bug 1321242.

Comment on attachment 8815562 [details] [diff] [review] bug1301055 - allow HLS video cannot be played twice without reloading the whole page (for aurora) fix fennec video issue, for aurora52

needs rebasing for aurora

Carry flag from comment98, it's used for aurora.

landed thanks!