Closed Bug 657791 Opened 14 years ago Closed 9 years ago

Seeking in WebM files with no Cues element is not supported

Categories

(Core :: Audio/Video: Playback, defect, P2)

Product:
Core
Component:
Audio/Video: Playback
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: cpearce, Assigned: bryce)

References

(Depends on 1 open bug)

Details

(Whiteboard: [caf priority: p2][CR 735038])

Attachments

(7 files, 13 obsolete files)

Testcase - short WebM file
(deleted), audio/webm
Details
Testcase - Webm video file without cues
(deleted), video/webm
Details
Testcase - Webm video file without cues and duration longer than frames
(deleted), video/webm
Details
Patch1 - Enable seeking in Cueless WebMs
(deleted), patch
jya
: review+
Details | Diff | Splinter Review
Patch2 - Clamp seeking in cueless files rather than failing
(deleted), patch
kinetik
: review+
Details | Diff | Splinter Review
Patch3-FixSeekableRanges.patch
(deleted), patch
Details | Diff | Splinter Review
Patch4-AddCuelessWebMTests.patch
(deleted), patch
Details | Diff | Splinter Review
Attached audio Testcase - short WebM file (deleted) — 
The attached short WebM audio only cannot be played more than once.

STR:
1. Load the attachment in a video document.
2. Wait until WebM file (automatically) plays to completion.
3. Click the "play" button. Audio should replay, but it does not.

I get the assertion failure:
5976[bb9ab18]: ###!!! ASSERTION: Should have audio start time by now: 'audioStartTime != -1', file c:/Users/cpearce/src/
mauve/objdir/content/media/../../../content/media/nsBuiltinDecoderStateMachine.cpp, line 420

I produced the file with `ffmpeg -i content/media/test/small-shot.ogg small-shot.webm`.
Looks like the seek is failing in nestegg_track_seek because the media doesn't contain a Cues element.
Longer files too:
http://pearce.org.nz/video/zurie.webm

Chrome is able to seek in such files, but it looks like they're doing a linear scan to perform the seek.
OS: Windows 7 → All
Summary: Can't replay short audio only WebM files → Can't seek in audio only WebM files (with no Cues)
Well, this bug ultimately reduces to "can't seek in any files without Cues", which we know.  The spec requires Cues, but it also requires them to be video only.  That wording needs to be fixed.

Ideally, all browsers would have approximately the same seeking behaviour, so we'll need to take this up with the other vendors and decide what to do.

Replaying a file could be a special case, though, because you can just restart the entire decoder.
I filed http://code.google.com/p/webm/issues/detail?id=332 to request that Cues explicitly be made mandatory in the audio-only case, and I've raised the issue on the WebM-discuss mailing list. In general they seem receptive to having Cues for audio, and want to not require seeking when there's no Cues, which is fine with me.

I think special casing replaying is a good idea however, even when there aren't Cues.
Summary: Can't seek in audio only WebM files (with no Cues) → Seeking in WebM files with no Cues element is not supported
Depends on: 575140
Whiteboard: [CR 735038]
Whiteboard: [CR 735038] → [caf priority: p2][CR 735038]
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.1?
NI :cperace here on what the effort is here to fix this in 2.1 given CAF is raising this a blocker for 2.1 in https://bugzilla.mozilla.org/show_bug.cgi?id=1082545#c22.

:bhargav, how common is this use case ?
Flags: needinfo?(cpearce)
Flags: needinfo?(bhargavg1)
(In reply to bhavana bajaj [:bajaj] from comment #9)
> NI :cperace here on what the effort is here to fix this in 2.1 given CAF is
> raising this a blocker for 2.1 in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1082545#c22.
> 
> :bhargav, how common is this use case ?

Also, we would also have to get an understanding of the risk this may add in addition to effort.
I would assume that WebM files without Cues (keyframe index) are uncommon. We've had this bug for a few years now, and the sky has not fallen.

Given all the other things we're working on at the moment, I cannot imagine we'll prioritize this, but that would be a question that the media team manager, Anthony Jones, should answer.
Flags: needinfo?(cpearce) → needinfo?(ajones)
(In reply to bhavana bajaj [:bajaj] from comment #9)
> NI :cperace here on what the effort is here to fix this in 2.1 given CAF is
> raising this a blocker for 2.1 in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1082545#c22.

Tell them no. I don't see a compelling reason to look into this.
Flags: needinfo?(ajones)
(In reply to bhavana bajaj [:bajaj] from comment #9)
> 
> :bhargav, how common is this use case ?

Hi Bhavana,
Use case is very much common. Its a seek using slider on video playback.
We see pauses while seek operation.
Flags: needinfo?(bhargavg1)
(In reply to Pooja from comment #13)
> (In reply to bhavana bajaj [:bajaj] from comment #9)
> > 
> > :bhargav, how common is this use case ?
> 
> Hi Bhavana,
> Use case is very much common. Its a seek using slider on video playback.
> We see pauses while seek operation.

Most WebM files have Cues. This bug only covers seeking in WebM files that do not provide seeking information. Those files are not common. WebM files normally provide seeking information.
No longer blocks: CAF-v2.1-CC-metabug
blocking-b2g: 2.1? → ---
Component: Video/Audio → WebRTC: Audio/Video
Priority: -- → P2
Component: WebRTC: Audio/Video → Video/Audio
Firefox itself produces WebM videos with no seeking information:

https://bugzilla.mozilla.org/show_bug.cgi?id=982573

I've also been seeing these in the wild lately from web apps using the WebP-sequence-to-WebM converter Whammy.  For example http://depthy.me/.  And since the people making these videos are using Chrome, they don't notice anything is wrong.

These should be fixed on the encoder end, but there will always be bad encoders.  Maybe it's not important for Firefox to be able to seek to arbitrary points in these videos, but would it be possible to at least make seeking to the beginning work?  As of now, looping via the loop attribute is broken, and I have to reload the page if I want to restart the video.
I work for a call center software company which heavily relies on audio recordings of customer interactions with agents. The ability to navigate (seek) these audio files while listening to them is a key feature of our product. This bug does impact us considerably.
Component: Audio/Video → Audio/Video: Playback
Assignee: nobody → bvandyk
Attached patch Patch1 - Enable seeking in Cueless WebMs (obsolete) (deleted) — — Splinter Review 

  

  



        Attachment #8713358 -
        Flags: review?(jyavenard)

    
    

    
  


  

  

  



        Attachment #8713359 -
        Flags: review?(jyavenard)

    
    

    
  


  

  

  



    
    

    
  


  

  

  



    
    

    
  
The first patch submitted should make WebMs seekable, however, there is an issue with the handling of duration in such WebMs. If one seeks a time beyond the end of the frames, the decoder state goes into error. This can be observed by applying the first patch and using the long duration testcase (stream the file and seek to near the end). This will also cause the test_seek test cases to intermittently fail on the no-cues.webm file which was added to these files tested.

The second patch addresses the issues present in the first. If seeking beyond the end of the frames, the demuxer should now clamp to the last block.

  

  



        Attachment #8713358 -
        Flags: review?(jyavenard) → review+

    
    

    
  
Comment on attachment 8713358 [details] [diff] [review]
Patch1 - Enable seeking in Cueless WebMs

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

nit: 
s/the the/that the/
s/seekeable/seekable/
s/are no considered seekable/are considered seekable

doesn't make much sense otherwise

  

  



    
    

    
  
Comment on attachment 8713359 [details] [diff] [review]
Patch2 - Clamp seeking in cueless files rather than failing

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

::: dom/media/webm/WebMDemuxer.cpp
@@ +713,1 @@
>      }

we are here because nestegg failed to seek, I'm not sure what failing should do.

I'll pass that decision to kinetik, he's more knowledgeable about webm internal structure than I am.

Can't nestegg be able to seek without cues?

  

  



        Attachment #8713359 -
        Flags: review?(jyavenard) → review?(kinetik)

    
    

    
  


  
Updating Patch1 to fix issues with commit message noted above.

  

  



        Attachment #8713358 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8713418 [details] [diff] [review]
Patch1 - Enable seeking in Cueless WebMs

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

Typically, getting r+ with comments or nit, means you are trusted to make the relevant changes and don't need to request for review once again.

removeing the r? or r- typically means you will need to re-apply for review.

Some remove the r? request, some do r-, it depends on the person.

  

  



        Attachment #8713418 -
        Flags: review?(jyavenard) → review+

    
    

    
  
Comment on attachment 8713359 [details] [diff] [review]
Patch2 - Clamp seeking in cueless files rather than failing

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

(In reply to Jean-Yves Avenard [:jya] from comment #29)
> we are here because nestegg failed to seek, I'm not sure what failing should
> do.
> 
> I'll pass that decision to kinetik, he's more knowledgeable about webm
> internal structure than I am.

Yeah, I'm not entirely sure about this new behaviour.  How do we handle seeking outside of valid ranges in MP4?

It seems like clamping the seek target should be handled before we get to this point, anyway.

> Can't nestegg be able to seek without cues?

No, it expects to find Cues (for nestegg_track_seek) or be provided a Cluster start offset (for nestegg_offset_seek).  Seeking to a specified timecode without Cues would require new code to perform a bisection search for the appropriate Cluster within the stream.

::: dom/media/webm/WebMDemuxer.cpp
@@ +708,5 @@
>      bool rv = mBufferedState->GetOffsetForTime(target, &offset);
>      if (!rv) {
> +      WEBM_DEBUG("mBufferedState->GetOffsetForTime failed too,\
> +                 using last block offset");
> +      offset = mBufferedState->GetLastBlockOffset();

Passing the start offset of the last Block parsed to nestegg_offset_seek is invalid and will fail with an error because nestegg_offset_seek expects to be given the start offset of a Cluster.

You want the WebMTimeDataOffset.mSyncOffset from the last block.  But see above about whether we even want to take this approach.

  

  



        Attachment #8713359 -
        Flags: review?(kinetik) → review-

    
    

    
  
Note that if we need to be able to seek in WebM streams (not just local files) without Cues, something along the of what I mentioned in paragraph 2 of comment 32 will be required, because the WebMBufferedState is only populated for data we have already received from the network.  It's always fully populated for local files because we read the entire file when we first query the WebMBufferedState, which is obviously not practical for remote streams.

  

  



    
    

    
  
(In reply to Matthew Gregan [:kinetik] from comment #32)

> Yeah, I'm not entirely sure about this new behaviour.  How do we handle
> seeking outside of valid ranges in MP4?

we never encounter that case.
mp4 always have a samples table. So we determine an exact image of the content when reading the metadata and have an accurate value of the file duration and where each samples are.

The MediaDecoderOwner will already clamp the seek value to always be within the seekable range (which by default is [0, duration)

So really, we can't seek outside valid ranges.

We pretty much always have "clues" in the other cases.
To determine where we should seek, we simply iterate over our sample tables and get an accurate position on which position in the MediaResource we have to get to.


> 
> It seems like clamping the seek target should be handled before we get to
> this point, anyway.

I think we should do it like what MP3 is doing when it doesn't have a VBR table.

It actually start demuxing the entire content to find where it needs to go.
It then builds an average bitrate value and estimate where approximately it should seek in the MediaResource. From that point it search for a new start of block.

This seems like the right thing to do here.

Seems to me that in Bryce test it worked because all of his files were fully cached, and had been parsed entirely by the WebMBufferedParser , but that won't always be the case, in particular if the webm file is big.

> 
> > Can't nestegg be able to seek without cues?
> 
> No, it expects to find Cues (for nestegg_track_seek) or be provided a
> Cluster start offset (for nestegg_offset_seek).  Seeking to a specified
> timecode without Cues would require new code to perform a bisection search
> for the appropriate Cluster within the stream.

then maybe P1 should be r- because it doesn't make sense then.

  

  



    
    

    
  
Are there any recommended steps to get WebM audio-only files working in Firefox in the meantime? I've tried the following with no luck. Cues element seems to be added to the file, but it still is unable to seek (using ffmpeg 2.8.4).

> ffmpeg -i sound1.wav -dash 1 sound1.webm

  

  



    
    

    
  
Disregard my last comment, once I restarted Firefox the updated file started working. May have just been a caching issue.

  

  



    
    

    
  


  
I've updated the patch based on the above feedback. Please let me know your thoughts.

  

  



        Attachment #8713359 -
        Attachment is obsolete: true

        Attachment #8714193 -
        Flags: review?(kinetik)

    
    

    
  
Comment on attachment 8714193 [details] [diff] [review]
Patch2 - Clamp seeking in cueless files rather than failing

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

HTMLMediaElement::Seek clamps the seek target to the seekable range.

Before your change in p1, that range would be empty because IsMediaSeekable() was false for files without Cues.

With your change in p1, that range will be 0-duration (or 0-infinity for infinite length streams).

However, without Cues, we can only complete seeks within the buffered range (until we implement Cue-less seeking) because we rely on mBufferedState to provide a time->offset lookup.  So I think the correct thing is for the seekable ranges to be the buffered ranges for WebM media that has no Cues but is backed by a seekable transport.

Right now, seekable == buffered is only true if the transport is not seekable.  So I think that needs to be addressed before p1 gives us the desired behaviour.

::: dom/media/webm/WebMBufferedParser.h
@@ +271,5 @@
>                                   uint64_t* aStartTime, uint64_t* aEndTime);
>  
>    // Returns true if aTime is is present in mTimeMapping and sets aOffset to
>    // the latest offset for which decoding can resume without data
>    // dependencies to arrive at aTime.

This comment is incorrect with respect to the actual behaviour when aTime is less than the first entry in mTimeMapping, and with the proposal of rolling GetOffsetForLastCluster's change into GetOffsetForTime will be even more incorrect, so please update it to document the actual behaviour.

::: dom/media/webm/WebMDemuxer.cpp
@@ +708,5 @@
>      bool rv = mBufferedState->GetOffsetForTime(target, &offset);
>      if (!rv) {
> +      WEBM_DEBUG("mBufferedState->GetOffsetForTime failed too,\
> +                 trying last cluster offset");
> +      rv = mBufferedState->GetOffsetForLastCluster(&offset);

Now that I've dug into this more, I think that instead of adding GetOffsetForLastCluster, we should just change the behaviour of GetOffsetForTime to return the last cluster's mSyncOffset in this situation.

While considering how to handle the case where target is less than the first timecode in mTimeMapping, it turns out we simply return the first mSyncOffset known (which could be arbitrarily far in the future).  So GetOffsetForTime's behaviour already handles half of this case, and I think it makes sense to update it to handle the other half rather than adding GetOffsetForLastCluster (which I think would need a comment where it's used to explain why it's being used).

@@ +711,5 @@
> +                 trying last cluster offset");
> +      rv = mBufferedState->GetOffsetForLastCluster(&offset);
> +      if (!rv) {
> +        WEBM_DEBUG("mBufferedState->GetOffsetForLastCluster failed too, \
> +                    returning");

We generally prefer to use quotes on each line with multi-line literals rather than escaping the newline, otherwise you are forced to use uglier formatting or introduce unwanted whitespace into the string. (this applies above too)

  

  



        Attachment #8714193 -
        Flags: review?(kinetik)
Blocks: 969290

    
    

    
  
Hi Bryce, may you give me a timeline for when the fix for this should show up in Nightly?

  

  



    
    

    
  
I'm investigating new tests to cover this functionality at the moment. Just an estimate, but I would hope to have the code side of this sorted out over the coming week barring any unexpected trouble.

  

  



    
    

    
  
Thanks a lot for the update Bryce

  

  



    
    

    
  
For comparison, I just tried this out in Chrome and Opera with support for experimental features enabled, and the recorded video does loop. Chrome 48, Opera 35.

  

  



    
    

    
  


  

  

  



        Attachment #8714193 -
        Attachment is obsolete: true

        Attachment #8718507 -
        Flags: review?(kinetik)

    
    

    
  


  

  

  



        Attachment #8718509 -
        Flags: review?(kinetik)

    
    

    
  


  

  

  



        Attachment #8718513 -
        Flags: review?(kinetik)

        Attachment #8718509 -
        Attachment is obsolete: true

        Attachment #8718509 -
        Flags: review?(kinetik)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch4-UpdateTestsToFetch.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

  

  



        Attachment #8718510 -
        Attachment is obsolete: true

        Attachment #8718510 -
        Flags: review?(kinetik)

        Attachment #8718514 -
        Flags: review?(kinetik)

    
    

    
  
Comment on attachment 8718514 [details] [diff] [review]
Patch4-UpdateTestsToFetch.patch

Found a couple of tests I think I didn't catch in my first rework. Obsoleting patch4 for now while I rework and retry them.

  

  



        Attachment #8718514 -
        Attachment is obsolete: true

        Attachment #8718514 -
        Flags: review?(kinetik)

        Attachment #8718507 -
        Flags: review?(kinetik) → review+

        Attachment #8718513 -
        Flags: review?(kinetik) → review+

    
    

    
  
I think it would have been more elegant to change IsSeekable() to actually return a seekable range.
We have some machinery to have a different seekable attribute between the MediaSourceDecoder, and the other reports the seekable range as being [0, duration).

All of this could be replaced with a single MediaDataDemuxer method. Would elegantly remove the need for custom MediaDecoder::GetSeekable() and handle this problem too.

BTW: There's a few spelling mistakes and typos in the commit log.

  

  



    
    

    
  
Comment on attachment 8718513 [details] [diff] [review]
Patch3 - Report only buffered ranage as seekable for cueless WebMs

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

::: dom/media/MediaDataDemuxer.h
@@ +71,5 @@
>    virtual bool IsSeekable() const = 0;
>  
> +  // Returns true if the underlying resource can only seek within buffered
> +  // ranges.
> +  virtual bool IsSeekableOnlyInBufferedRanges() const = 0;

Please don't make this one pure virtual, have a default implementation that return false. so you don't have to modify all those files and you only need to worry about the WebMDemuxer

  

  



    
    

    
  


  
Rework previous patch to prefer non-abstract IsSeekableOnlyInBufferedRanges().

  

  



        Attachment #8718513 -
        Attachment is obsolete: true

        Attachment #8719347 -
        Flags: review?(kinetik)

    
    

    
  
Comment on attachment 8719347 [details] [diff] [review]
Patch3 - Report only buffered ranage as seekable for cueless WebMs

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

In the commit log:

> changes to make cuelss webms playable, a 4th option is needed: a file that is

cueless

And the last paragraph of the commit log can probably be left out entirely.

(In reply to Jean-Yves Avenard [:jya] from comment #62)
> I think it would have been more elegant to change IsSeekable() to actually
> return a seekable range.
> We have some machinery to have a different seekable attribute between the
> MediaSourceDecoder, and the other reports the seekable range as being [0,
> duration).
> 
> All of this could be replaced with a single MediaDataDemuxer method. Would
> elegantly remove the need for custom MediaDecoder::GetSeekable() and handle
> this problem too.

Agree that would be nicer.  Bryce, can you please file a follow-up bug for someone to do this refactoring?

  

  



        Attachment #8719347 -
        Flags: review?(kinetik) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch3-FixSeekableRanges.patch (deleted)
        — 
          — Splinter Review
      

    


  

  

  



        Attachment #8719347 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Bryce Van Dyk (:SingingTree) from comment #70)
> Created attachment 8719352 [details] [diff] [review]
> Patch3-FixSeekableRanges.patch

Fix typo and remove last paragraph.

  

  



    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch4-AddCuelessWebMTests (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
This patch implements a subset of the behaviour exercised by the seek tests, but for cueless WebMs. I had originally written this into the existing seek tests, but decided it complicated the seek tests and that I'd split these out on their own. Once random access seeking in implemented for cueless WebMs the distinction between the tests will make removal of these ones very straight forward.

I have noticed troubling behaviour while running precursors to these tests in try. I've had fails that I cannot reproduce locally, and that I'm uncertain as to the cause of. I have an inkling something may be going on with races in the event listeners the tests are using, but as noted, cannot repro. This set of tests appears to be playing nice in try, but I am still weary. As such, I'd appreciate any input regarding the logic of the tests.

  

  



        Attachment #8719631 -
        Flags: review?(kinetik)

        Attachment #8719631 -
        Flags: review?(jyavenard)

    
    

    
  
Comment on attachment 8719631 [details] [diff] [review]
Patch4-AddCuelessWebMTests

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

if you are copying this file from another, please ensure that the commit reflect that. Now if you're using git it's going to be a problem and you need to call git diff with -C and -M. With hg use hg copy blah blah2.html

I'm going to r- this on the proviso that you aren't testing what you describe in the comment.

You can only seek in the buffered range. Yet nowhere do you check if the video has been buffered yet. This isn't something trivial to do seeing that it's rather random.
I can bet that if you were to run this on try, you *will* get intermittent failures.
In particular on b2g platforms where autoload is set to "metadata" only, so "loadeddata" won't be fired.

canplaythrough is fired once we've estimated that the download speed appears to allow playing without interruption. But this doesn't mean that the video is fully buffered and that you can seek to duration / 2.

per spec, when seeking, if seekable doesn't contain the target, it will seek to the closest point. So the test currentTime - halfDuration < 0.1 is not guaranteed to always succeed.

::: dom/media/test/mochitest.ini
@@ +613,5 @@
>  [test_closing_connections.html]
>  [test_constants.html]
>  [test_controls.html]
> +[test_cueless_webm_seek-1.html]
> +skip-if = android_version == '10' # bug 1059116

any reason to skip on android? don't see how the bug linked to is relevant

::: dom/media/test/test_cueless_webm_seek-1.html
@@ +48,5 @@
> +      v.seeking = true;
> +      readonly = v.seeking === false;
> +    }
> +    catch(e) {
> +      readonly = "threw exception: " + e;

this seems to be out of the scope of this mochitest... testing that the seeking attribute is writeable that is. 
Plus now you've stored a string into readonly, and testing that it's true just below

Please remove all those read-only tests.

@@ +58,5 @@
> +  }
> +
> +  function seekStarted() {
> +    ok(!completed, "should not be completed yet");
> +    ok(Math.abs(v.currentTime - halfDuration) < 0.1,

this is a racy test. After seeking event is handled, nothing guarantees that currentTime wouldn't have moved passed the seeking duration.

My guess that is the reason why the original test was intermittently failing on some platforms.

When you seek, currentTime immediately shows the new logical position

@@ +64,5 @@
> +    startPassed = true;
> +  }
> +
> +  function seekEnded() {
> +    ok(!completed, "shuld not be completed yet");

should not be completed

  

  



        Attachment #8719631 -
        Flags: review?(jyavenard) → review-

    
    

    
  
I'm not sure I follow the original comment. The patch file itself is new, I've scrapped the old ones, is there a convention there with following? Or is there a file in the patch which is out of order?

> You can only seek in the buffered range. Yet nowhere do you check if the
> video has been buffered yet. This isn't something trivial to do seeing that
> it's rather random.
> I can bet that if you were to run this on try, you *will* get intermittent
> failures.
> In particular on b2g platforms where autoload is set to "metadata" only, so
> "loadeddata" won't be fired.

Is there a better way to do this? The test_buffered does the same thing and fetches the target file and assumes that it will be buffered by the time events are fire. Is there a difference in these cases?

Is b2g a concern? I was under the impression that is it is no longer so.

> canplaythrough is fired once we've estimated that the download speed appears
> to allow playing without interruption. But this doesn't mean that the video
> is fully buffered and that you can seek to duration / 2.

As above. Is there a better way to do this? I figured this is the most restrictive event to wait on and followed the same pattern as used in test_buffered.

> ::: dom/media/test/mochitest.ini
> @@ +613,5 @@
> >  [test_closing_connections.html]
> >  [test_constants.html]
> >  [test_controls.html]
> > +[test_cueless_webm_seek-1.html]
> > +skip-if = android_version == '10' # bug 1059116
> 
> any reason to skip on android? don't see how the bug linked to is relevant

I did this as test_seek-2 is set to skip on android and the same functionality is exercised.

> ::: dom/media/test/test_cueless_webm_seek-1.html
> @@ +48,5 @@
> > +      v.seeking = true;
> > +      readonly = v.seeking === false;
> > +    }
> > +    catch(e) {
> > +      readonly = "threw exception: " + e;
> 
> this seems to be out of the scope of this mochitest... testing that the
> seeking attribute is writeable that is. 
> Plus now you've stored a string into readonly, and testing that it's true
> just below

Here I'm exercising the same functionality as test_seek-1. My intention was to replicate the same things here. It can be removed, however if that is the case is there any reason why it should be in test_seek and not here?

> 
> @@ +58,5 @@
> > +  }
> > +
> > +  function seekStarted() {
> > +    ok(!completed, "should not be completed yet");
> > +    ok(Math.abs(v.currentTime - halfDuration) < 0.1,
> 
> this is a racy test. After seeking event is handled, nothing guarantees that
> currentTime wouldn't have moved passed the seeking duration.
> 
> My guess that is the reason why the original test was intermittently failing
> on some platforms.

So this patterns such as this should be corrected in the original test_seeks also?

> When you seek, currentTime immediately shows the new logical position
> 
> @@ +64,5 @@
> > +    startPassed = true;
> > +  }
> > +
> > +  function seekEnded() {
> > +    ok(!completed, "shuld not be completed yet");
> 
> should not be completed

I'm not sure I follow here.

  

  



    
    

    
  
(In reply to Bryce Van Dyk (:SingingTree) from comment #77)
> I'm not sure I follow the original comment. The patch file itself is new,
> I've scrapped the old ones, is there a convention there with following? Or
> is there a file in the patch which is out of order?

Didn't you modify an existing mochitest?

if so , the history should show it. Makes it easier to understand what's going on.
so if you copied seek-1.html , it shouldn't be a new file, but a copy of seek-1.html and then modify it.

> 
> > You can only seek in the buffered range. Yet nowhere do you check if the
> > video has been buffered yet. This isn't something trivial to do seeing that
> > it's rather random.
> > I can bet that if you were to run this on try, you *will* get intermittent
> > failures.
> > In particular on b2g platforms where autoload is set to "metadata" only, so
> > "loadeddata" won't be fired.
> 
> Is there a better way to do this? The test_buffered does the same thing and
> fetches the target file and assumes that it will be buffered by the time
> events are fire. Is there a difference in these cases?

I haven't looked at that mochitest, but the event you test do not guarantee that the file is fully buffered, nor that the seek target (duration / 2) is in the buffered range.

This is what you need to test.

Testing the buffered attribute to check that it contains the seek target is all you need to do.

Also add in your test that the seekable attribute is == buffered attribute; as if it's different something is wrong and the gecko code is buggy.

> 
> Is b2g a concern? I was under the impression that is it is no longer so.

sure, but writing something that you know won't work isn't gonna help :)
and in any case, the issue is the same on android (preload is metadata by default, not auto like on desktop)

> 
> > canplaythrough is fired once we've estimated that the download speed appears
> > to allow playing without interruption. But this doesn't mean that the video
> > is fully buffered and that you can seek to duration / 2.
> 
> As above. Is there a better way to do this? I figured this is the most
> restrictive event to wait on and followed the same pattern as used in
> test_buffered.

the file being small, canplaythrough is typically going to indicate that you have fully loaded the file.
But that's only of preload=auto. Otherwise the MediaResource will only download enough to read the metadata and won't buffer anything.

Testing the buffered attribute and ensuring it's also identical to the seekable attribute is an acceptable compromise.

> > any reason to skip on android? don't see how the bug linked to is relevant
> 
> I did this as test_seek-2 is set to skip on android and the same
> functionality is exercised.

then there's no need to skip the test on android :)

> Here I'm exercising the same functionality as test_seek-1. My intention was
> to replicate the same things here. It can be removed, however if that is the
> case is there any reason why it should be in test_seek and not here?

ok then.


> 
> > 
> > @@ +58,5 @@
> > > +  }
> > > +
> > > +  function seekStarted() {
> > > +    ok(!completed, "should not be completed yet");
> > > +    ok(Math.abs(v.currentTime - halfDuration) < 0.1,
> > 
> > this is a racy test. After seeking event is handled, nothing guarantees that
> > currentTime wouldn't have moved passed the seeking duration.
> > 
> > My guess that is the reason why the original test was intermittently failing
> > on some platforms.
> 
> So this patterns such as this should be corrected in the original test_seeks
> also?

in the other tests, the entire file is seekable, not just the buffered range!
it's a fundamental difference between the two.

say you start your test and the buffered range is [0 , 1] the file is 4s long you seek to 2.
Per spec seeking to 2, will end up seeking to 1, so currentTime - halfDuration is > .1

> > should not be completed
> 
> I'm not sure I follow here.

spelling: should not shuld

  

  



    
    

    
  
> Didn't you modify an existing mochitest?

Ah, I see. I had created entirely new files and pieced them together from other tests.
 
> I haven't looked at that mochitest, but the event you test do not guarantee
> that the file is fully buffered, nor that the seek target (duration / 2) is
> in the buffered range.
> 
> This is what you need to test.
> 
> Testing the buffered attribute to check that it contains the seek target is
> all you need to do.

Is there a way to have the tests wait for a buffered state? I am unaware of any event being fired, but without that I figured I'd need to poll, which seems like a smell.

> Also add in your test that the seekable attribute is == buffered attribute;
> as if it's different something is wrong and the gecko code is buggy.

Will do.
 
> the file being small, canplaythrough is typically going to indicate that you
> have fully loaded the file.
> But that's only of preload=auto. Otherwise the MediaResource will only
> download enough to read the metadata and won't buffer anything.
> 
> Testing the buffered attribute and ensuring it's also identical to the
> seekable attribute is an acceptable compromise.

What I'm really after is a test friendly way of making sure the data is buffered (as above). I had reasoned that test buffered would provide a template for doing so. However, based on the discussion here, it sounds as though even if the media is fetched first, there is still no assurance that it will be buffered.
 
> > > 
> > > @@ +58,5 @@
> > > > +  }
> > > > +
> > > > +  function seekStarted() {
> > > > +    ok(!completed, "should not be completed yet");
> > > > +    ok(Math.abs(v.currentTime - halfDuration) < 0.1,
> > > 
> > > this is a racy test. After seeking event is handled, nothing guarantees that
> > > currentTime wouldn't have moved passed the seeking duration.
> > > 
> > > My guess that is the reason why the original test was intermittently failing
> > > on some platforms.
> > 
> > So this patterns such as this should be corrected in the original test_seeks
> > also?
> 
> in the other tests, the entire file is seekable, not just the buffered range!
> it's a fundamental difference between the two.
> 
> say you start your test and the buffered range is [0 , 1] the file is 4s
> long you seek to 2.
> Per spec seeking to 2, will end up seeking to 1, so currentTime -
> halfDuration is > .1

Is your follow up comment related to the original race one? I believe I understand the issue noted in the follow up comment. I took the original comment to mean that it was possible for current time to have incremented after the seek. Is this due to playback and seek events racing? If that is the case, this could happen regardless of if the file has cues or not, right? So in any test file, so long as playback is not paused, checking currentTime after a seek introduces a race into the test?

> spelling: should not shuld

Cool, will fix this here and in the test_seek it's based on.

  

  



        Attachment #8719631 -
        Flags: review?(kinetik)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch4-AddCuelessWebMTests.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Update the previous patch following discussions with jya and others. The tests now poll the buffered state, which is ugly, but I believe should remove the races injected by waiting on available events.

I've removed code such as this:
> +  }
> +
> +  function seekStarted() {
> +    ok(!completed, "should not be completed yet");
> +    ok(Math.abs(v.currentTime - halfDuration) < 0.1,
After looking at the spec (https://dev.w3.org/html5/spec-preview/media-elements.html#seeking), it appears that the seeking event is fired, and could be acted upon, before the currentTime is updated (so depending on the impl this races, if I understand correctly).

Also included:
- Removed android test skip
- Each test explicitly checks that seekable's start and end are == buffered's start and end
-Fixed typo 'shuld' -> 'should'

  

  



        Attachment #8719631 -
        Attachment is obsolete: true

        Attachment #8720201 -
        Flags: review?(jyavenard)

    
    

    
  
(In reply to Bryce Van Dyk (:SingingTree) from comment #83)
> After looking at the spec
> (https://dev.w3.org/html5/spec-preview/media-elements.html#seeking), it
> appears that the seeking event is fired, and could be acted upon, before the
> currentTime is updated (so depending on the impl this races, if I understand
> correctly).

(prefer https://html.spec.whatwg.org/multipage/embedded-content.html#seeking it's far more up to date)

not sure if I'm reading you correctly, but seeking event is QUEUED for dispatch before the currentTime is updated (step 10), so as far as JS is concerned it's irrelevant to you as this is done before changing the currentTime attribute returns and by then the seeking event has not yet been dispatched, you're still in the same event loop.

when changing currentTime, currentTime is immediately updated.

However, as per step 8: "If the (possibly now changed) new playback position is not in one of the ranges given in the seekable attribute, then let it be the position in one of the ranges given in the seekable attribute that is the nearest to the new playback position. If two positions both satisfy that constraint (i.e. the new playback position is exactly in the middle between two ranges in the seekable attribute) then use the position that is closest to the current playback position. If there are no ranges given in the seekable attribute then set the seeking IDL attribute to false and abort these steps."

so no, you can't always act upon and assume that currentTime == seekTarget once you receive "seeking" by then seeking has already started, and may have completed or not.
And seeing that seekable == buffered the actual currentTime may have been updated to a value that was in the seekable range.

As I mentioned over IRC, I don't see why you would need to poll nor what it would add: you can't control what is being buffered unless you use MSE.
So polling never guarantees that you will always get the buffered range you seek (pun intended).
Based on that predicament, simply waiting for an event such as loadedmetadata which you are guaranteed to get (if you don't invoke play()). Testing then what the buffered range is and seeing that seeking in a position contained in the buffered range succeed. is all you need.

In the tests where you do invoke play() then you will always get loadeddata event and canplay and can use the buffered range when you are receiving those events.

  

  



    
    

    
  
Comment on attachment 8719352 [details] [diff] [review]
Patch3-FixSeekableRanges.patch

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

::: dom/media/MediaDecoder.h
@@ +263,4 @@
>    bool IsMediaSeekable();
> +  // Returns true if this media supports seeking only in buffered ranges. True
> +  // for example in WebMs with no cues
> +  bool IsMediaSeekableOnlyInBufferedRanges();

I was thinking about this on my way to work.

None of this SeekableOnlyInBufferedRanges is entirely necessary.

Rather than modifying the base implementation of GetSeekable, it would be much more elegant to override WebMDecoder::GetSeekable instead.

We do need to have a seekable mirror setup like buffered is.

  

  



    
    

    
  
(In reply to Jean-Yves Avenard [:jya] from comment #84)

> As I mentioned over IRC, I don't see why you would need to poll nor what it
> would add: you can't control what is being buffered unless you use MSE.
> So polling never guarantees that you will always get the buffered range you
> seek (pun intended).
> Based on that predicament, simply waiting for an event such as
> loadedmetadata which you are guaranteed to get (if you don't invoke play()).
> Testing then what the buffered range is and seeing that seeking in a
> position contained in the buffered range succeed. is all you need.
> 

After discussing with roc and cpearce I decided to implement the polling option.

The problem I had with listening for loadedmetadata is it races if any data is that it races if the mediaElement.buffered is populated. Running the tests previously using that I've seen passes, but also fails where buffered has no elements so there is nothing to seek within.

I'm currently trying a commit using loadeddata. Will update patch if that plays nice in try.

(In reply to Jean-Yves Avenard [:jya] from comment #85)
> 
> Rather than modifying the base implementation of GetSeekable, it would be
> much more elegant to override WebMDecoder::GetSeekable instead.
> 
> We do need to have a seekable mirror setup like buffered is.

Are you suggesting I rework the patch? If so should we just implement the fix needed for Bug 1248306 instead?

  

  



    
    

    
  
(In reply to Bryce Van Dyk (:SingingTree) from comment #86)
> 
> I'm currently trying a commit using loadeddata. Will update patch if that
> plays nice in try.
> 

This appears to still race. I'm not sure I understand why, but the media element does not appear to have buffered.end(0) even after loadeddata is fired.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4359563a5274&selectedJob=16867978

  

  



    
    

    
  
(In reply to Bryce Van Dyk (:SingingTree) from comment #86)
> Are you suggesting I rework the patch? If so should we just implement the
> fix needed for Bug 1248306 instead?

it would be better, otherwise a follow up patch will do. Can do that in the same bug kinetik asked you top opened above to have IsSeekable return a TimeIntervals instead of a boolean

  

  



    
    

    
  
(In reply to Jean-Yves Avenard [:jya] from comment #88)
> (In reply to Bryce Van Dyk (:SingingTree) from comment #86)
> > Are you suggesting I rework the patch? If so should we just implement the
> > fix needed for Bug 1248306 instead?
> 
> it would be better, otherwise a follow up patch will do. Can do that in the
> same bug kinetik asked you top opened above to have IsSeekable return a
> TimeIntervals instead of a boolean

Might I suggest we land this in its current state? I agree that the polling approach is non-ideal, but based on our discussions here and on IRC it appears we cannot rely on buffered being populated after events firing due to races. I also agree that returning a range is a good idea, but think that we should discuss some details further on the associated bug. As such, I think it'd be worthwhile to have this landed if there are no objections, as it provides an improvement on functionality, and as there is a follow up ticket to cover the improvement for seekable range.

  

  



    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch4-AddCuelessWebMTests.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Update the tests and MediaFormatReader after discussion with :cpearce. The change to MediaFormatReader ensures that the buffered ranges will be updated after metadata is loaded and the demuxer is initialised. This prevents a race where the buffered attribute of a media element may or may not be populated following loadeddata being fired.

This means the tests can now listen for such an event, rather than polling.

  

  



        Attachment #8720201 -
        Attachment is obsolete: true

        Attachment #8720201 -
        Flags: review?(jyavenard)

        Attachment #8723748 -
        Flags: review?(jyavenard)

    
    

    
  
(In reply to Bryce Van Dyk (:SingingTree) from comment #91)
> Created attachment 8723748 [details] [diff] [review]
> Patch4-AddCuelessWebMTests.patch
> 
> Update the tests and MediaFormatReader after discussion with :cpearce. The
> change to MediaFormatReader ensures that the buffered ranges will be updated
> after metadata is loaded and the demuxer is initialised. This prevents a
> race where the buffered attribute of a media element may or may not be populated 
> following loadeddata being fired.

I had a look a couple of days about it, and right when the demuxer initialisation is done https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#291
GetBuffered() is called.

WebMDemuxer::NotifyDataArrived only set mNeedReIndex to true.

The first thing WebMDemuxer::GetBuffered() does is called EnsureUpToDateIndex() which will update the buffered range.

mNeedReIndex is initialised with true in the constructor so recalculation will always be done upon the first call to GetBuffered()

the buffered attribute is using a tail dispatch, so it's always run before anything else.

so adding a call to NotifyDataArrived() in MediaFormatReader will certainly not fix any races, at best it will only reduces the chance of it because you're queuing another task for later. 

So the reason you are seeing an empty attribute isn't for the lack of call to NotifyDataArrived(). I'm afraid that's not a sufficient fix.
There's an issue in the WebMDemuxer and this what needs fixing.

(If changes were to be made in MediaFormatReader, they are to be made in a separate patch thank you)

  

  



    
    

    
  
Comment on attachment 8723748 [details] [diff] [review]
Patch4-AddCuelessWebMTests.patch

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

r=me for the tests only.

Please separate the fix for the buffered range attribute being empty when loadeddata is fired.

First, it's not NotifyDataArrived() that needs to be called; it worked because it calls UpdateBuffered(). 


We don't want to do this in AsyncReadMetadata(), because there's no guarantee we'll have a non-empty buffered attribute when reading the metadata has completed. The demuxer may have retrieved just that.

What we need to make sure of is that buffered attribute is non empty when loadeddata is called.
So we must ensure that MediaDecoderReader::Update() is called prior MediaDecoder::FirstFrameLoaded being called.

The best place to do this in the MDSM where it handles the first frame decoding.
Problem here is that UpdateBuffered() is being run on the reader's taskqueue while FirstFrameLoaded is called on the main thread. So we can't just dispatch a task to run UpdateBuffered() and be guaranteed that FirstFrameLoaded will be called after.

I think we'll have to add a new method like GetBufferedWithPromise, that will be resolved once GetBuffered() is called, so that the MediaDecoderStateMachine will complete FirstFrameLoaded once GetBuffered() has been run.

I'm guess this will fix some intermittents we are seeing in the mochitests too.

IMHO that deserves its own bug number.

::: dom/media/MediaFormatReader.cpp
@@ +347,5 @@
> +  /* Notify data arrived to ensure that the buffered ranges correctly reflect
> +  the information we have now that metadata is loaded and the demuxer is
> +  initialized. This prevents a race where it's possible to have a media element
> +  fire loadeddata, but not yet have its buffered attribute populated. */
> +  NotifyDataArrived();

It's UpdateBuffered() that needs to be called.
There's no race as such so I think this comment is unnecessary.

Please take this into another patch.
see general comment about it.

::: dom/media/test/test_cueless_webm_seek-1.html
@@ +38,5 @@
> +  var seekFlagEnd = false;
> +  var readonly = true;
> +  var completed = false;
> +
> +  var halfBuffered = v.buffered.end(0) / 2;

add ok(v.buffered.length >= 1) here to make sure that it's indeed buffered

@@ +102,5 @@
> +
> +  var loaded = function (event) {
> +    if (xhr.status == 200 || xhr.status == 206) {
> +      // Request fulfilled. Note sometimes we get 206... Presumably because either
> +      // httpd.js or Necko cached the result.

I guess you mean Gecko

@@ +118,5 @@
> +  return function(uri) {
> +    var v = document.createElement('video');
> +    v._token = token;
> +    v.src = uri;
> +    v.addEventListener("loadeddata", testWebM1, false);

would have been nicer to use promises and once() => ...
but I guess that will do.

::: dom/media/test/test_cueless_webm_seek-2.html
@@ +35,5 @@
> +  var startPassed = false;
> +  var endPassed = false;
> +  var completed = false;
> +
> +  var halfBuffered = v.buffered.end(0) / 2;

same as the other:
have ok(v.buffered.length >= 1)

::: dom/media/test/test_cueless_webm_seek-3.html
@@ +35,5 @@
> +  var startPassed = false;
> +  var completed = false;
> +  var gotTimeupdate = false;
> +
> +  var halfBuffered = v.buffered.end(0) / 2;

ok(v.buffered.length >= 1)

  

  



        Attachment #8723748 -
        Flags: review?(jyavenard) → review+
Depends on: 1251460

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch4-AddCuelessWebMTests.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Updated tests to remove the onfetched generator function which is no longer needed. Added check to make sure buffered is populated once loadeddata has been fired.

In regards to the 'Necko' in the comments, that's the same as the original buffered test comment. I was under the impression it's referring to https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Necko Please let me know if that's not the case and I'll update the comments.

  

  



        Attachment #8723748 -
        Attachment is obsolete: true

    
    

    
  


  
Improve logging on new ok() calls from last patch.

  

  



        Attachment #8724598 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch5-WaitOnBufferedPromiseForLoadedData.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Based on dicussion wtih :jya rework MediaDecoderStateMachine::EnqueueFirstFrameLoadedEvent to make an async call to MediaDecoderReader to update buffered. Following the return of the promise from this call MDSM will perform the logic from EnqueueFirstFrameLoadedEvenet. This aims to remedy a race where the buffered ranges may or may not be updated once loadeddata has fired.

  

  



        Attachment #8725498 -
        Flags: review?(jyavenard)

    
    

    
  
Comment on attachment 8725498 [details] [diff] [review]
Patch5-WaitOnBufferedPromiseForLoadedData.patch

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

Also please move this to bug 1251460..

as it deserves a fix on its own.

thank you

::: dom/media/MediaDecoderReader.h
@@ +233,5 @@
>      NotifyDataArrivedInternal();
>      UpdateBuffered();
>    }
>  
> +  RefPtr<BufferedUpdatePromise> UpdateBufferedWithPromise() {

MOZ_ASSERT(OnTaskQueue());

we want all our methods to include this to maintain thread safety and code clarity

@@ +234,5 @@
>      UpdateBuffered();
>    }
>  
> +  RefPtr<BufferedUpdatePromise> UpdateBufferedWithPromise() {
> +	  UpdateBuffered();

unlike your earlier solution where you didn't have to worry about recalculating the buffered range, here you do want to ensure that the buffered range has been updated, otherwise the value you will read (at least with WebMDemuxer) is of the buffered attribute at the time the metadata were loaded.

I can't see a nice way to do this other than calling NotifyDataArrived()
instead

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2043,5 @@
> +        MediaDecoderEventVisibility visibility =
> +        self->mSentFirstFrameLoadedEvent ? MediaDecoderEventVisibility::Suppressed
> +        : MediaDecoderEventVisibility::Observable;
> +        self->mFirstFrameLoadedEvent.Notify(nsAutoPtr<MediaInfo>(new MediaInfo(self->mInfo)),
> +        Move(visibility));

indentation isn't good. Please use the same identation as was there before (just shifted to the right)

@@ +2044,5 @@
> +        self->mSentFirstFrameLoadedEvent ? MediaDecoderEventVisibility::Suppressed
> +        : MediaDecoderEventVisibility::Observable;
> +        self->mFirstFrameLoadedEvent.Notify(nsAutoPtr<MediaInfo>(new MediaInfo(self->mInfo)),
> +        Move(visibility));
> +        self->mSentFirstFrameLoadedEvent = true;

I think you want mSentFirstFrameLoadedEvent to be set prior this, as we have code that rely on this value to decide if we've completed decoding of the first frame.

You also only need to worry about this for the case where mSentFirstFrameLoadedEvent is false.

This function is called when you're coming back from dormant, in which case we don't need to worry about the buffered range being updated.

So the UpdateBufferedWithPromise only needs to be called when mSentFirstFrameLoadedEvent is false

in which case it would go something like:

if (!mSentFirstFrameLoadedEvent) {
  mSentFirstFrameLoadedEvent = true;
  InvokeAsync(DecodeTaskQueue(), mReader.get(), __func__, ... 
    { self->mFirstFrameLoadedEvent.Notify(nsAutoPtr<MediaInfo>(new MediaInfo(self->mInfo)), MediaDecoderEventVisibility::Observable)
} else {
  mFirstFrameLoadedEvent.Notify(nsAutoPtr<MediaInfo>(new MediaInfo(self->mInfo)), MediaDecoderEventVisibility::Suppressed) 
}

  

  



        Attachment #8725498 -
        Flags: review?(jyavenard)

        Attachment #8725498 -
        Attachment is obsolete: true

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/4261e08ba51a
https://hg.mozilla.org/mozilla-central/rev/2e990a8d4553
https://hg.mozilla.org/mozilla-central/rev/fd6e5a3c2817
https://hg.mozilla.org/mozilla-central/rev/51766683141b

  

  


Status: NEW → RESOLVED
Closed: 9 years ago

          status-firefox48:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
No longer blocks: 1198715

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: