Video from m.youtube.com is not playing

VERIFIED FIXED in Firefox 19

Status

P1
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: nhirata, Assigned: djf)

Tracking

({late-l10n, regression})

unspecified
B2G C3 (12dec-1jan)
ARM
Gonk (Firefox OS)
late-l10n, regression
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

1. launch browser
2. go to http://www.youtube.com/watch?v=e_qHyj9-CWs
3. tap on the video

Expected: video plays
Actual: nothing happens

Note: 1/13/2012 build. unagi
Triage: BB+, C3, P2 - regression
blocking-basecamp: ? → +
Keywords: regression
Priority: -- → P2
Target Milestone: --- → B2G C3 (12dec-1jan)
Created attachment 692838 [details]
Error when getting video info

Please note that this issue is not a general case.
From my test, it should be OK to play some other videos on YouTube.

For this specific video, http://www.youtube.com/watch?v=e_qHyj9-CWs, the root cause that it could not be played back,

  1. We would try to get the video info from Gecko's b2g/components/YoutubeProtocolHandler.js with this url, http://www.youtube.com/get_video_info?&video_id=e_qHyj9-CWs

  2. In this case it would return failure, as the attachment.

--
We need to consider error handling for youtube video playing.
Duplicate of this bug: 822303
(Assignee)

Comment 4

6 years ago
I'm working on a couple of related bugs, so I'll take this one also.

At a minimum, I'll make the video app display an error message if the video fails to play.  If time allows, I'll do something more intelligent with the get_video_info API that Rudy describes in comment 2.
Assignee: nobody → dflanagan
(Assignee)

Comment 5

6 years ago
Actually, I'm not going to take this one. This needs to be fixed in gecko, as Rudy said, and I can't do anything about it in Gaia.

It seems clear how to detect the error, and even how to extract an error message. I just don't know enough about gecko protocol handlers to know how to report an error in a useful way.  Nor do I know what the browser would do when that error occurs.

If someone can help with that info, I'm willing to have this bug assigned to me, but there may be others who can handle it better than I can.
Assignee: dflanagan → nobody
(Assignee)

Comment 6

6 years ago
Hmm.  Maybe I can handle this in Gaia.  If the protocol handler gets an error when calling the get_video_info API, that error needs to be displayed to the user somehow. If the browser doesn't have any good way to display this error, then we should make the activity handler that plays youtube videos display youtube-related errors.  So maybe the error message should just be passsed in the activity request.
Assignee: nobody → dflanagan
OS: Mac OS X → Gonk (Firefox OS)
Priority: P2 → P1
Hardware: x86 → ARM

Updated

6 years ago
Duplicate of this bug: 824561

Comment 8

6 years ago
Hi, David, are you still working on this? Thanks!
(Assignee)

Comment 9

6 years ago
(In reply to khu from comment #8)
> Hi, David, are you still working on this? Thanks!

I haven't actually started on it, but plan to. I think the fix is that if youtube isn't willing to let us display the video, we pass the error message in the view activity to the video player, and have the video display the error message instead of the video.

(Unless there is some other way to get an error message from the youtube protocol handler back to the browser to be displayed in some nice way...)

Even though this is a P1, I kind of need to fix bug 815791 and bug 815826 (both P3s) before I can fix this one. That's what the hold up has been.
(Assignee)

Comment 10

6 years ago
b2g/components/YoutubeProtocolHandler.js is kind of a mess.  It is not parsing the video title properly which explains why we sometimes get a title and sometimes do not.

I'm fixing that along with my fix to check the status code of the result from youtube. If youtube won't give us an http: URL for the video, then we'll pass the error code and reason string to the activity handler for display there.

I've got a patch that I'm in testing now.
Keywords: late-l10n
(Assignee)

Comment 11

6 years ago
I've flagged this as late-l10n and cc'ed Stas because if youtube won't let us play the video we need to display an error message of some sort.

Youtube sends us an error message like this: "This video contains content from Sony ATV Publishing. It is restricted from playback on certain sites." I've seen reports online of this error message appearing in other languages, so hopefully it will be appropriately localized.

But we still need a title of some sort that is guaranteed to be localized. It needs to give the message "we can't play this video" and also the message "it is youtube's fault, not ours".

Maybe "Can't play video. Youtube says:"
(Assignee)

Comment 12

6 years ago
Created attachment 696442 [details] [diff] [review]
patch for b2g/components/YoutubeProtocolHandler.js

This is the gecko-side patch for this bug.  It modifies YoutubeProtocolHandler to check the status parameter and pass any youtube error messages on to the view activity so they can be displayed to the user.

It also fixes the code to property parse the title and thumbnail images.  The parsing code is consolidated into the nested parseYoutubeVideoInfo() function.
Attachment #696442 - Flags: review?(dale)
(Assignee)

Comment 13

6 years ago
Created attachment 696447 [details]
link to patch on github

This is the gaia-side piece of the fix: if youtubeprotocolhandler sends an error message, the video view activity will display that message (using alert() for simplicity) instead of playing a video.

See the notes on github: the pull request includes two commits. The first is for bug 815791. The second commit is for this bug and is what needs review here.
Attachment #696447 - Flags: review?(dale)
Comment on attachment 696442 [details] [diff] [review]
patch for b2g/components/YoutubeProtocolHandler.js

+    function log(msg) {
+      msg = "YoutubeProtocolHandler.js: " + (msg.join ? msg.join(" ") : msg);

Does e.message ever pass an array? either way I dont think the join logic belongs in the log function

+        if (x < y) 
+          return 1;
+        else if (y > x) 
+          return -1;
+        else 
+          return 0;

x and y are both integers here so I think |return x - y| is the idiomatic way to sort them, but not a big deal.

Both are nits so can land without, rest is good by me
Attachment #696442 - Flags: review?(dale) → review+
Blocks: 809188
(Assignee)

Comment 15

6 years ago
Thanks for those comments... I copied the log function from ContentHandler.js or something like that, since dump() wasn't working for me.  And I don't know what I was thinking about the array sorting function. I'll fix that.

Do you know whether I need to update the UUID of this file for a change like this?
Do you mean the Components.ID? as far as I know that is only needed when creating new components so I dont think so, but whoever checks it in can confirm
(Assignee)

Updated

6 years ago
Attachment #696447 - Flags: review?(stas)
(Assignee)

Comment 17

6 years ago
Dale - it looks like you merged the gaia patch, but didn't mark r+ here.  Would you do that please?  Since this has landed, it means that youtube titles won't display at all until the gecko patch lands.

Stas - I've asked you to review the gaia patch (that was just merged) because it includes a new string as described in comment 11.  Thanks.  Hope I'm doing this late-l10n stuff right!
(Assignee)

Comment 19

6 years ago
Dale: no, I mean the third attachment. This one: https://bugzilla.mozilla.org/attachment.cgi?id=696447

You reviewed it on github and merged it, but didn't mark r+ here.
Attachment #696447 - Flags: review?(dale) → review+
Oh, sorry about that, done
(Assignee)

Comment 21

6 years ago
Created attachment 696583 [details] [diff] [review]
fix b2g/components/YoutubeProtocolHandler.js, v2

This is a new version of the gecko patch, with a simplified sort comparator function.  Its a cosmetic change only and I've tested this new version.
Attachment #696442 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/eb2f5c66561b
https://hg.mozilla.org/releases/mozilla-aurora/rev/f46562ce9da7
https://hg.mozilla.org/releases/mozilla-b2g18/rev/319add03fbf4
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-b2g18: --- → fixed
status-firefox19: --- → fixed
status-firefox20: --- → fixed
Resolution: --- → FIXED

Comment 23

6 years ago
Comment on attachment 696447 [details]
link to patch on github

Grabbing the review request from stas.

AFAICT, the string should use sentence case, as it's not a button? See https://etherpad.mozilla.org/gaia-copy-guidelines
Attachment #696447 - Flags: review?(stas) → review-

Comment 24

6 years ago
PS: I'm not going to migrate the string to gaia-l10n for now, so we can fix the string without key change still.
(Assignee)

Comment 25

6 years ago
My thought was that the youtube-error-prefix string would serve as a sort of title to the error message that youtube sends us, so that is why I started each word with a capital and used two newlines to separate it from the youtube error message.  (Ideally, I'd use a more sophisticated UI than the alert() function to display the title and the message, but there isn't time.)

I'll change to lowercase and add a colon at the end so the string reads:

   YouTube won't play video:

Does that seem better?

I suppose I should open a new bug for this change, though.
(Assignee)

Updated

6 years ago
Blocks: 825522
(Assignee)

Comment 26

6 years ago
I've moved the final l10n string issue to new bug 825522

Comment 27

6 years ago
To close the discussion both here and in the follow-up:

Titles are sentence-case, so yes, the new string is better. Just buttons with actionable verbs are camel case, and should only be one or two words.
Busted. See bug 825698.

Comment 29

6 years ago
Verified that videos that Youtube does not allow to be displayed on certain devices are now giving the user the following error message, "Well, this is embarrassing. We tried to display this Web page, but it's not responding." with a Reload button underneath. 
Checked on Build 20130103070201 on 1/3/13
Status: RESOLVED → VERIFIED
(Assignee)

Comment 30

6 years ago
Chris,

Actually what you describe is a symptom of bug 825698. You get that for all youtube videos right now.

You should see a different message, but you won't be able to verify until that other bug is fixed.

Comment 31

6 years ago
Returning issue to it's Resolved state until bug 825698 has been resovled.
Status: VERIFIED → RESOLVED
Last Resolved: 6 years ago6 years ago

Comment 32

6 years ago
Created attachment 701639 [details]
Error msg

Comment 33

6 years ago
David, 
Please see the attachment on comment #32. Are you talking about this error msg.

On unagi Build ID: 20130113070202 v 1.0.0-Prerelease
(Assignee)

Comment 34

6 years ago
Deepa,

The screenshot you've attached is verification that this bug is resolved.  You tried to play a video that youtube won't let us play. Previously there was no explaination. Now we display the youtube error to the user so they know what is going on.

The error mentioned in comments 29 and 30 was a different error, caused by a different bug, now resolved.

All is good here!

Comment 35

6 years ago
Verified On unagi Build ID: 20130113070202 v 1.0.0-Prerelease
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.