Closed
Bug 677122
Opened 13 years ago
Closed 6 years ago
Changing media fragment portion of URL in video documents doesn't update start/end times
Categories
(Core :: Audio/Video: Playback, defect)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
INCOMPLETE
People
(Reporter: cajbir, Assigned: scott.downe)
References
Details
Attachments
(1 file, 10 obsolete files)
6.33 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
When navigating directly to a video, changing the media fragment portion of the URL doesn't change the start/end times of the video. You need to refresh the page to get this to happen. Steps to reproduce: 1) Open http://www.bluishcoder.co.nz/mediafrag/AudioAPI.webm#t=50,100 2) Change the fragment portion to be "#t=30.100" and press enter Expected Results: 1) Start and End fragment of the video would be the new position of 30 and 100 respectively. Actual Results: 1) The video does not change the start/end fragments. They stay at 50 and 100.
Reporter | ||
Updated•13 years ago
|
Summary: Changing media fragment portion of URL in video documents doesn't update play position → Changing media fragment portion of URL in video documents doesn't update start/end times
Updated•12 years ago
|
Assignee: nobody → scott.downe
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•12 years ago
|
||
I got this working with a reload of the document. Not currently able to seek the video as needed without having to reload, may need help with that :) It was interesting to not that a media fragment string "#t=20,100" would go into the anchor handling code, this chokes that off. By choking that off, it will reload the page because it has nothing else to do, which in turn will reload the video and thus call ProcessMediaFragment. My first patch, so any comments on simple process or style related things is appreciated.
Attachment #592407 -
Flags: feedback?(david.humphrey)
Attachment #592407 -
Flags: feedback?(chris.double)
Assignee | ||
Comment 2•12 years ago
|
||
It just hit me, this will not work. Not only is #t a valid anchor, #t=60,50 is also a valid anchor, so checking against the string is a silly idea.
Assignee | ||
Comment 3•12 years ago
|
||
I also fixed a small typo in a comment I modified, and moved a variable into the only scope it is used in.
Attachment #592407 -
Attachment is obsolete: true
Attachment #592407 -
Flags: feedback?(david.humphrey)
Attachment #592407 -
Flags: feedback?(chris.double)
Attachment #595603 -
Flags: review?
Assignee | ||
Updated•12 years ago
|
Attachment #595603 -
Flags: review? → review?(bzbarsky)
Comment 4•12 years ago
|
||
Comment on attachment 595603 [details] [diff] [review] Don't do a short-circuited load on synthetic documents This would violate step 7 of http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#navigating-across-documents and the http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#scroll-to-fragid it links to. What we need to do here is to "perform some other action, such that the indicated part of the document is brought to the user's attention" without changing the document object. Most simply, would adding a script to the <head> (allowed per http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#read-media ) that watches for hashchange events and adjusts things as needed work?
Attachment #595603 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 5•12 years ago
|
||
Thanks for the review! I have a patch with an attempt at your approach. There are two things I really like about this approach, one being it is in JavaScript which is really awesome! The other being how modular it is, so it is pretty safe that nothing can break because of this. One thing I don't like is how it is reproducing the code for media fragments twice. There is already c++ code to parse this, and things would be more maintainable if we could simply tap into that at some point. Ideally call the parseMediaFragment code via the hashchange js event, but I doubt the js can call the mediaFragmentParser. That is my thinking anyway. I also had problems finding a good place to put the mediaFragment.js file. I am going to continue to look into that, but I feel putting this out there now as is and getting some feedback is crucial.
Attachment #595603 -
Attachment is obsolete: true
Attachment #600490 -
Flags: review?(bzbarsky)
Comment 6•12 years ago
|
||
Hrm. Duplicating the parser in the JS isn't quite what I had in mind.... Does just changing media.src to be the new document URI not in the hashchange listener do the right thing by any chance? If it doesn't, it's worth talking to some of the media folks about good ways to do this. Ideally we could just pass off the work to all the existing C++ code. As far as where to put the file... one option is to use an inline script altogether. Esp. if the script ends up short enough. ;) Speaking of which, you could do: var media = document.querySelector("audio,video"); to get the media element.
Assignee | ||
Comment 7•12 years ago
|
||
I actually don't see a src on the video element in a media doecument, but, adding one, and changing it does give us the desired functionality. I did have it inline, but I explored it as a file too. It felt too long the way it was. If we can get it as short and simple as: "document.querySelector("audio,video")[0].src = newSrc;" inside the hashchange then inline would be perfect. I will give this a go. Thanks again.
Comment 8•12 years ago
|
||
> I actually don't see a src on the video element in a media doecument It has one internally, just not in an attribute. > "document.querySelector("audio,video")[0].src = newSrc;" Yeah, if that works, that sounds good. ;)
Assignee | ||
Comment 9•12 years ago
|
||
Awesome, starting to take shape :)
Attachment #600490 -
Attachment is obsolete: true
Attachment #600490 -
Flags: review?(bzbarsky)
Attachment #600607 -
Flags: review?(bzbarsky)
Comment 10•12 years ago
|
||
Comment on attachment 600607 [details] [diff] [review] reload src element on hashchange for media documents >+++ b/content/html/document/src/MediaDocument.cpp >+nsresult No one checks the return value... just return void? >+MediaDocument::InsertMediaFragmentScript() >+ nsCOMPtr<nsINodeInfo> nodeInfo; Please declare it right next to the code that assigns it? >+ nsRefPtr<nsGenericHTMLElement> script = NS_NewHTMLScriptElement(nodeInfo.forget()); >+ NS_ENSURE_TRUE(script, NS_ERROR_OUT_OF_MEMORY); NS_NewHTML*Element never return null, so no need to check the return value. Which is good, because this code would leak on null return if it could happen! >+ script->SetInnerHTML(NS_LITERAL_STRING("window.addEventListener(\"hashchange\",function(e){document.querySelector(\"audio,video\").src=e.newURL;},false);")); SetTextContent, please. If you use single quotes in your JS, you don't have to escape them. >+ script->SetAttr(kNameSpaceID_None, nsGkAtoms::type, NS_LITERAL_STRING("text/javascript"), true); No need for that; scripts with no type set default to JavaScript. > MediaDocument::SetScriptGlobalObject(nsIScriptGlobalObject* aGlobalObject) >+ if (aGlobalObject && !nsContentUtils::IsChildOfSameType(this)) { The IsChildOfSameType check is wrong. It would make changes to the hash not work in subframes. When you write tests for this, make sure to test that? The check was there for the stylesheet because we don't want our "full page media" styling in subframes, in fact. r=me with those nits fixed. Thanks for doing this!
Attachment #600607 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 11•12 years ago
|
||
Sorry for the delay; I had problems with NS_ENSURE_TRUE returning nsresult, took a break, then got some help. Was exactly what I needed. I replaced the NS_ENSURE_TRUE with an if statement and a void return. Not 100% confident we event need it there. Thanks for the review.
Attachment #600607 -
Attachment is obsolete: true
Attachment #604410 -
Flags: review?(bzbarsky)
Comment 12•12 years ago
|
||
Comment on attachment 604410 [details] [diff] [review] reload media document on media fragment hash change r=me Would be good to add a test if you can figure out how, of course. And please add a commit message and From line to the patch before requesting checkin!
Attachment #604410 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 13•12 years ago
|
||
Added a mochitest. Not quite sure where to go from here. Is there a two part review process? Should I be passing this on to someone else? Either way, I think Boris should take one last look, sanity check on the test I added :) Thanks!
Attachment #604410 -
Attachment is obsolete: true
Attachment #609185 -
Flags: review?(bzbarsky)
Comment 14•12 years ago
|
||
Comment on attachment 609185 [details] [diff] [review] reload media document on media fragment hash change The commit comment should actually say what's changing... there's no reloading happening here. r=me with that fixed. Thanks for the test! What happens next, after you fix the checkin comment, is you add the checkin-needed keyword to this bug. There _is_ a 2-part review process, but only for changes that change APIs and whatnot; for most things a single review is enough.
Attachment #609185 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 15•12 years ago
|
||
Attachment #609185 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 16•12 years ago
|
||
Comment on attachment 609568 [details] [diff] [review] Update start/end times of mediafragment URL on hashchanges Carrying forward r+ from bz.
Attachment #609568 -
Flags: review+
Comment 18•12 years ago
|
||
Backed out for blowing up the tree. https://hg.mozilla.org/integration/mozilla-inbound/rev/ed17cccf91cf http://30.media.tumblr.com/tumblr_m11ykqEl0P1rrf1eeo1_500.jpg
Comment 19•12 years ago
|
||
Sample log: https://tbpl.mozilla.org/php/getParsedLog.php?id=10419024&tree=Mozilla-Inbound PROCESS-CRASH | /tests/dom/tests/mochitest/dom-level2-html/test_HTMLFrameElement01.html | application crashed (minidump found) Crash dump filename: /tmp/tmps60MVV/minidumps/43051c7b-f3d7-38b2-36a4d674-1e9059ba.dmp Operating system: Linux 0.0.0 Linux 2.6.31.5-127.fc12.i686.PAE #1 SMP Sat Nov 7 21:25:57 EST 2009 i686 CPU: x86 GenuineIntel family 6 model 23 stepping 10 2 CPUs Crash reason: SIGSEGV Crash address: 0x0 Thread 0 (crashed) 0 libxul.so!nsINode::AppendChildTo [nsINode.h : 579 + 0x3] eip = 0x0112f564 esp = 0xbf9a6164 ebp = 0x00000000 ebx = 0x0227cc60 esi = 0x00000000 edi = 0xbf9a61b8 eax = 0x00000000 ecx = 0x9b406d84 edx = 0x00000000 efl = 0x00010292 Found by: given as instruction pointer in context 1 libxul.so!mozilla::dom::MediaDocument::InsertMediaFragmentScript [MediaDocument.cpp : 382 + 0xe] eip = 0x014318ef esp = 0xbf9a6180 ebp = 0x9b406c00 ebx = 0x0227cc60 esi = 0xbf9a61bc edi = 0xbf9a61b8 Found by: call frame info 2 libxul.so!mozilla::dom::ImageDocument::SetScriptGlobalObject [ImageDocument.cpp : 366 + 0x8] eip = 0x0142f5d2 esp = 0xbf9a61e0 ebp = 0x9b406c00 ebx = 0x0227cc60 esi = 0x9b406c00 edi = 0x9d61f2b8 Found by: call frame info 3 libxul.so!nsGlobalWindow::SetNewDocument [nsGlobalWindow.cpp : 2301 + 0x16] eip = 0x014c6991 esp = 0xbf9a6270 ebp = 0x9b406c00 ebx = 0x0227cc60 esi = 0x9d61c180 edi = 0xa0a22540 Found by: call frame info 4 libxul.so!DocumentViewerImpl::InitInternal [nsDocumentViewer.cpp : 980 + 0x14] eip = 0x01164679 esp = 0xbf9a63d0 ebp = 0x99b44050 ebx = 0x0227cc60 esi = 0x9b95a418 edi = 0x00000001 Found by: call frame info 5 libxul.so!DocumentViewerImpl::Init [nsDocumentViewer.cpp : 723 + 0x18] eip = 0x01164c5a esp = 0xbf9a6470 ebp = 0x9b95a400 ebx = 0x0227cc60 esi = 0x9b95a400 edi = 0x80004005 Found by: call frame info 6 libxul.so!nsDocShell::SetupNewViewer [nsDocShell.cpp : 7714 + 0x17] eip = 0x017b77ed esp = 0xbf9a64a0 ebp = 0x9b95a400 ebx = 0x0227cc60 esi = 0x9b95a400 edi = 0x80004005 Found by: call frame info 7 libxul.so!nsDocShell::Embed [nsDocShell.cpp : 5820 + 0xb] eip = 0x017bde0a esp = 0xbf9a66b0 ebp = 0x80004005 ebx = 0x0227cc60 esi = 0x9b95a400 edi = 0x99b44050 Found by: call frame info 8 libxul.so!nsDocShell::CreateContentViewer [nsDocShell.cpp : 7500 + 0x1a] eip = 0x017c0a93 esp = 0xbf9a66e0 ebp = 0x80004005 ebx = 0x0227cc60 esi = 0x9b95a400 edi = 0x9bb90830 Found by: call frame info 9 libxul.so!nsDSURIContentListener::DoContent [nsDSURIContentListener.cpp : 164 + 0x14] eip = 0x017c5b92 esp = 0xbf9a6760 ebp = 0x9bb90830 ebx = 0x0227cc60
Comment 20•12 years ago
|
||
Er, yes. Image documents, unlike video documents, don't create the synthetic document until their SetScriptGlobalObject is called... So we probably need to either null-check the GetHeadElement(), or just put this code into VideoDocument (where it would make more sense anyway), not MediaDocument.
Assignee | ||
Comment 21•12 years ago
|
||
If we put it into videoDocument, would it still work for audio? Also, I am requesting try server https://bugzilla.mozilla.org/show_bug.cgi?id=739873 to help put out that fire :)
Comment 22•12 years ago
|
||
> If we put it into videoDocument, would it still work for audio?
Yes. VideoDocument is used for both audio and video. It should perhaps be named MediaDocument, but there's already a MediaDocument (used for audio, video, images, and plug-is).
Assignee | ||
Comment 23•12 years ago
|
||
Perfect. I only had it in mediaDocument because I thought that meant video+audio. I will give that a try.
Assignee | ||
Comment 24•12 years ago
|
||
Everything is working with it on the videoDocument. I am still waiting on try server access, but I have the updated patch, and I'll see if I can get someone to run it through for me.
Attachment #609568 -
Attachment is obsolete: true
Attachment #613071 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 25•12 years ago
|
||
A small update to the test to get consistent results. This was exposed by running it through the try server. These are the results now from try: https://tbpl.mozilla.org/?tree=Try&rev=1b63bb1867e8 Are the oranges acceptable and known? Not sure how to make sense of all this, but the old failures are gone.
Attachment #613071 -
Attachment is obsolete: true
Attachment #613071 -
Flags: review?(bzbarsky)
Attachment #613219 -
Flags: review?(bzbarsky)
Updated•12 years ago
|
Keywords: checkin-needed
Comment 26•12 years ago
|
||
Comment on attachment 613219 [details] [diff] [review] Moved media fragment hash change code off mediaDocument and onto videoDocument r=me
Attachment #613219 -
Flags: review?(bzbarsky) → review+
Comment 27•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/14715fd6b81c Had to un-bitrot VideoDocument.cpp a bit. Please check it to make sure I didn't screw anything up.
Keywords: checkin-needed
Comment 28•12 years ago
|
||
Sorry, I had to back this out on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/f03124b00990 because the test was failing intermittently but frequently: https://tbpl.mozilla.org/php/getParsedLog.php?id=10810377&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=10810913&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=10810466&tree=Mozilla-Inbound Someone will need to figure out and fix the test failures before this can re-land. Please let us know if you need any help with that!
Assignee | ||
Comment 29•12 years ago
|
||
There are two issues here. One is the "Expected number of tests run. Expected: 5 Actual: 1", where the video I test against goes into what looks to be streaming mode. I can only reproduce this inside the mochi test. It is pretty elusive, but once you have a failing case, you can refresh the mochi test and it will continue to fail. The other one, the "Assertion failure: !IsNull() (Cannot compute with a null value), at ../../dist/include/mozilla/TimeStamp.h:245" I have not seen yet. I have a breakpoint there, and hope to catch it, but... this one stumps me. I have no idea where that is being called from. I hope, and suspect this is because of my inexperience in writing mochitests, and it is my test itself that is broken.
Assignee | ||
Comment 30•12 years ago
|
||
Oh, and the "Assertion failure: !IsNull() (Cannot compute with a null value), at ../../dist/include/mozilla/TimeStamp.h:245" passes my first test, so it is crashing after that test, but before the next.
Comment 31•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/14715fd6b81c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 32•12 years ago
|
||
Backed out: https://hg.mozilla.org/mozilla-central/rev/f03124b00990
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 33•12 years ago
|
||
Does the log not have a stack for the assertion failure?
Assignee | ||
Comment 34•12 years ago
|
||
Yeah, there is a stack. I am attempting to understand it now. Thread 25 (crashed) 0 linux-gate.so + 0x424 eip = 0x00d81424 esp = 0xa34fdfe0 ebp = 0xa34fdff8 ebx = 0x0000088c esi = 0x00c59844 edi = 0x00c7cff4 eax = 0x00000000 ecx = 0x00000930 edx = 0x00000006 efl = 0x00000202 Found by: given as instruction pointer in context 1 firefox-bin!MOZ_Crash [Assertions.cpp : 90 + 0x6] eip = 0x0804b2ad esp = 0xa34fe000 ebp = 0xa34fe018 Found by: previous frame's frame pointer 2 firefox-bin!MOZ_Assert [Assertions.cpp : 104 + 0x4] eip = 0x0804b2f4 esp = 0xa34fe020 ebp = 0xa34fe038 ebx = 0x0804ca84 Found by: call frame info 3 libxul.so!mozilla::TimeStamp::operator- [TimeStamp.h : 245 + 0x1f] eip = 0x0149f12e esp = 0xa34fe040 ebp = 0xa34fe068 ebx = 0x02bf728c esi = 0x0c0fd9e8 Found by: call frame info 4 libxul.so!nsBuiltinDecoderStateMachine::AdvanceFrame [nsBuiltinDecoderStateMachine.cpp : 1999 + 0x15] eip = 0x01a84680 esp = 0xa34fe070 ebp = 0xa34fe148 ebx = 0x02bf728c esi = 0x0c0fd9b8 edi = 0xa34fe0f8 Found by: call frame info mValue of the timestamp is null when we attempt a: TimeStamp presTime = mPlayStartTime - UsecsToDuration(mPlayDuration) + UsecsToDuration(currentFrame->mTime - mStartTime); It is the currentFrame->mTime - mStartTime that causes it. It is line 1999 nsBuiltinDecoderStateMachine.cpp. This goes into the minus operator, which fails because mValue is null.
Comment 35•12 years ago
|
||
Sounds like either currentFrame->mTime or mStartTime never got initialized. Which one of the to is it?
Assignee | ||
Comment 36•12 years ago
|
||
I am pretty sure it is currentFrame->mTime, based on what I now know about operator overloading. The operator fails because the TimeStamp local mValue is null, which should be the left hand side of the operation, thus the currentFrame->mTime.
Assignee | ||
Comment 37•12 years ago
|
||
Interesting, there is a if (currentFrame) { right before the call that fails. Based on that logic, I wonder if there would be any repercussions to also checking that !currentFrame.mTime.IsNull() So we only do the check if there is a currentFrame and the frame's time is not null. We don't want something uninitialized running around, that never will be initialized. Just... thoughts.
Assignee | ||
Comment 38•12 years ago
|
||
I am stumped on this. Matt Brubeck, I am going to take you up on the help offer :) I am convinced looking at the stack traces that the fails are due to the mochitest itself. But, I am having a hard time proving that. Help with creating the mochitest would be great, or at the least a review of what I have, to see if there are better ways to test what I am testing. The "Expected number of tests run. Expected: 5 Actual: 1" fail is due to the video going into what looks to be streaming, where the playhead is always at the end of the timeline, and you can only seek to a time that has already been played, making media fragment tests on a videoDocument impossible when it does this. It only does this sometimes with my test, though. I can reproduce what I mean all the time with this reduced test case: <!DOCTYPE HTML> <html> <head> <script type="text/javascript"> var url = "../../../media/test/seek.ogv"; window.open(url); </script> </head> <body> </body> </html> This has to be run as a mochitest to cause the streaming mode, but it happens every time. I don't see another way to test functionality of videoDocuments other than iframes of window.open. The Assertion failure issue stumps me more, but I am also pretty sure it is either a bug in my test, or a deeper bug I am exposing, but not causing. What makes me think this is in the stack trace where it failed, my first test passes, which I would suspect would not even happen if my patch was introducing the bug. I am going to try and find some existing tests for the videoDocument, see what those look like. maybe a mochitest is the wrong approach?
Assignee | ||
Comment 39•12 years ago
|
||
Changing the url in the above case to a remote one stops the streaming. It has something to do with the url not working while local in a mochitest.
Assignee | ||
Comment 40•12 years ago
|
||
A mochitest local url is http://mochi.test:8888/tests/content/media/test/seek.ogv It is the mochi.test:8888 that I am interested in. It is the only difference between this, and running it outside a mochitest, or remote urls.
Comment 41•12 years ago
|
||
Files at http://mochi.test are served by a simple web server written in JavaScript: https://mxr.mozilla.org/mozilla-central/source/testing/mochitest/server.js Is it possible that limitations or performance of this HTTP server are interfering with the test?
Assignee | ||
Comment 42•12 years ago
|
||
I am going to reduce my test to what I think causes these problems, and run that in try without the rest of my patch. See if it still fails, so we'll know if it is limitations or my patch. I will also take a look at that server.
Assignee | ||
Comment 43•12 years ago
|
||
I've reduced my test to what is causing the problem. I have a test that I would expect to pass, that will fail every time, on a clean branch. https://tbpl.mozilla.org/?tree=Try&rev=2c47a9772f56 https://hg.mozilla.org/try/rev/2c47a9772f56 This is an example of the streaming issue I was talking about. My test tries to trigger a mediafragment seek, and listens for it. It never happens, instead plays to the end and I listen for that, to fail the test. I believe this is in mochitest/server.js, and I am moving my attention there. Should this be filed in a new ticket? or just fix it here? Am I crazy to think this test should pass?
Comment 44•12 years ago
|
||
If is fails if the file is served by the mochitest framework, but not from a 'normal' http server, then it's better to open a separate bug for that.
Chris Double is on holiday right now, he'll be back in a couple of weeks. If you can wait that long, he's the best person to help here.
Assignee | ||
Comment 46•12 years ago
|
||
What's the possibility of landing this without tests? I don't think it is testable at this point. In comment 12 it was mentioned it would be good if I could figure out how to write a test for this... I cannot. I filed a bug on mochitests: https://bugzilla.mozilla.org/show_bug.cgi?id=747584 Without my tests this is the try result I get: https://tbpl.mozilla.org/?tree=Try&rev=14bb8473d1a1 There are a few errors in andoid that I don't know what to make of. At this point, I am up for any ideas or options, as I am out of them.
Comment 47•12 years ago
|
||
If you run the parts of the test async instead of sync, does that help?
Assignee | ||
Comment 48•12 years ago
|
||
I'm not sure what you mean by sync and async. I am running the tests inside event listeners. Is that what you mean by async? Is this a mochitest specific thing? Like an async mode? I did actually just have an idea. We don't test the functionality directly, but test the existence and content of the script element I inserted. That is an option that is better than nothing. I am curious as to what you mean by asynch, though.
Comment 49•12 years ago
|
||
I meant replacing the places where you call next() with: // Give whatever the media code is doing right now a chance to complete // before we seek. SimpleTest.executeSoon(next);
Assignee | ||
Comment 50•12 years ago
|
||
Cool. I am trying that solution: https://tbpl.mozilla.org/?tree=Try&rev=873fac3250a4 *fingers crossed* what does executeSoon do anyway? Does it wait until everything has stopped? Boris, if this works, you would be a hero! Our efforts are not for naught though, as I have filed two bugs while trying to track this down.
Comment 51•12 years ago
|
||
> what does executeSoon do anyway?
Posts an event to the event loop to call the function when the event fires. The idea being to not do the seek until the event handler you're in right now has returned and the stack unwound.
If that fixes things, we may need another bug filed unless the two you filed cover things already, because it's not good when web content triggers asserts. ;)
You could put the <iframe> and initial src directly into your HTML file instead of creating and inserting it via the DOM. That would sipmlify things a little bit. I don't think your "seeked" event handler is quite right. The current time might have advanced past test.start by the time the "seeked" handler runs. Are you expecting to get a "pause" *and* "ended" event when we reach the end of the first interval? If so, why are we expecting count to be 3 in "ended"? Seems to me it would only be 2.
Assignee | ||
Comment 53•12 years ago
|
||
I initially had the iframe in the HTML, but that is one of the leading causes of the streaming issues. That makes it happen every time, as opposed to some times. My example in the bug I filed has the iframe in the html. Now, it might solve my problem if I put the src also in the html, but then I may miss my load event. The load event might fire before I can get the reference to the iframe and setup an event. Ideas for that? You're most likely right on the seeked bit. I will change the time comparisons to greater than or equal to, see if that helps. That way if it advances a bit past, it won't be a problem. The first test is the range of 1-2, second being 3-4. 4 is the end of the video. The 3-4 range actually triggers an end, and not a pause, where as the 1-2 triggers a pause. This also means if the functionality breaks, and the seeked and pause don't happen, the end still happens, and the tests fail, instead of timing out. I would take improvements on this, if you have some, though. Thanks for the feedback.
Comment 54•12 years ago
|
||
> Ideas for that?
<script>
function foo() {
// do stuff
}
</script>
<iframe onload="foo()" src="whatever"></iframe>
Assignee | ||
Comment 55•12 years ago
|
||
Sweet, running those suggestions now: https://tbpl.mozilla.org/?tree=Try&rev=5a3f409f1844 I have good feelings about it. The other bug I filed is here: https://bugzilla.mozilla.org/show_bug.cgi?id=745035 I was attempting to reproduce the assertion error, and came up with the above. The assertion crash stacks, a reminder, for reference, and because we don't want to lose them: https://tbpl.mozilla.org/php/getParsedLog.php?id=10810377&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=10810466&tree=Mozilla-Inbound Not convinced yet they are the same, but they may be related. I have yet to see it myself though, other than the above stacks. There is definitely something weird going on with the loading and order of mediadocument mediafragments and iframes. Those stacks are still all we have from that.
Assignee | ||
Comment 56•12 years ago
|
||
The SimpleTest.executeSoon(next) doesn't quite do it, still got a seek error. https://tbpl.mozilla.org/php/getParsedLog.php?id=11086360&tree=Try That tells me the end was fired, and no other tests run, with seek and pause being skipped because it is streaming. Still waiting on the iframe in html with onload="load()".
Assignee | ||
Comment 57•12 years ago
|
||
Test now uses both SimpleTest.executeSoon(next) and iframe is in the html with an onload. I've run it through try multiple times and have yet to see a test fail or assertion error. I want to try landing this again :)
Attachment #613219 -
Attachment is obsolete: true
Attachment #617254 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 58•12 years ago
|
||
Ignore that last one. I was testing something, so I had commented out code that was not intended in the diff. Everything else should be the same.
Attachment #617254 -
Attachment is obsolete: true
Attachment #617254 -
Flags: review?(bzbarsky)
Attachment #617265 -
Flags: review?(bzbarsky)
Comment 59•12 years ago
|
||
Comment on attachment 617265 [details] [diff] [review] Update start/end times of mediafragment URL on hashchanges r=me but please make sure we have a bug tracking those test failures when the next() was synchronous!
Attachment #617265 -
Flags: review?(bzbarsky) → review+
Updated•12 years ago
|
Keywords: checkin-needed
Comment 60•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a01bb9f10445
Keywords: checkin-needed
Target Milestone: mozilla14 → mozilla15
Comment 61•12 years ago
|
||
Sorry, I backed this out again: https://hg.mozilla.org/integration/mozilla-inbound/rev/ee6e8470cb96 because the test failed with: test_bug677122.html | Expected number of tests run. Expected: 5 Actual: 1 https://tbpl.mozilla.org/php/getParsedLog.php?id=11175908&tree=Mozilla-Inbound
Target Milestone: mozilla15 → ---
Assignee | ||
Comment 62•12 years ago
|
||
Still not enough eh. Question, is there a way to easily run this through try more than once? I ask, because it doesn't fail often, and can have clean runs, only to have it fail later. After a fix, to be sure, I would like to run it multiple times.
Comment 63•12 years ago
|
||
You can restart jobs from the tbpl page (look for the + icon in the bottom right pane) or from the self-serve API.
Updated•9 years ago
|
Component: Audio/Video → Audio/Video: Playback
Comment 64•6 years ago
|
||
There's been no activity on this patch in 6 years, so I assume it won't apply cleanly now. If the issue is still present and you want to work on your fix more, please feel free to re-open.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 6 years ago
Resolution: --- → INCOMPLETE
Comment 65•4 years ago
|
||
I still experience this bug in 81.0.
You need to log in
before you can comment on or make changes to this bug.
Description
•