Bug 24849 – Implement HTMLMediaElement 'played' attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Eric Carlson]

HTMLMediaElement currently returns an empty TimeRanges object for 'played'

[Pierre d'Herbemont]

Created attachment 29452 [details]
Proposed patch v1.

Attached a proposed patch.

[Pierre d'Herbemont]

Created attachment 29453 [details]
patch v2.

(this time without LayoutTests/media/video-played_2.html)

[Pierre d'Herbemont]

Created attachment 29471 [details]
patch v3.

(Previous test case was garbaged for some obscure reasons.)

[Pierre d'Herbemont]

Created attachment 29486 [details]
patch v4.

Removed a left over (printf) introduced in v3.

[Simon Fraser (smfr)]

Comment on attachment 29471 [details]
patch v3.


>      if (m_readyState == HAVE_NOTHING || !m_player) {
> +        printf("HTMLMediaElement::seek INVALID_STATE_ERR\n");

You have an extraneous printf here

> diff --git a/WebCore/html/TimeRanges.cpp b/WebCore/html/TimeRanges.cpp
> index ad81ac8..2d69229 100644

> +    unsigned int overlappingArcIndex;
> +    Range addedRange(start, end);
> +
> +    for (overlappingArcIndex = 0; overlappingArcIndex < m_ranges.size(); overlappingArcIndex++) {
> +        if (addedRange.isOverlappingRange(m_ranges[overlappingArcIndex])) {
> +            addedRange = addedRange.unionWithOverlappingRange(m_ranges[overlappingArcIndex]);
> +            m_ranges.remove(overlappingArcIndex);
> +            overlappingArcIndex--;
> +        } else {
> +            // Check the case for which there is no more to do
> +            if (!overlappingArcIndex) {
> +                if (addedRange.isBeforeRange(m_ranges[0]))
> +                    break;
> +            } else {
> +                if (m_ranges[overlappingArcIndex - 1].isBeforeRange(addedRange)
> +                    && addedRange.isBeforeRange(m_ranges[overlappingArcIndex])) {
> +                    break;
> +                }
> +            }
> +        }
> +    }
> +
> +    // Now that we are sure we don't overlap with any arc, just add it.
> +    m_ranges.insert(overlappingArcIndex, addedRange);

How confident are you that this handles all possible range configurations? Range math is tricky.

> +        inline bool isPointInRange(float point)
> +        {
> +            return m_start <= point && point <= m_end;
> +        }

I think this should be 
  return m_start <= point && point < m_end;

so you'll also have to test for contiguous ranges in your amalgamation code.

> +        inline bool isOverlappingRange(Range range)
> +        {
> +            return isPointInRange(range.m_start) || isPointInRange(range.m_end) || range.isPointInRange(m_end);
> +        }

Are you sure this handles both types of entirely nested ranges?

> +        inline Range unionWithOverlappingRange(Range range)
> +        {
> +            Range ret;
> +
> +            ret.m_start = fminf(m_start, range.m_start);
> +            ret.m_end = fmaxf(m_end, range.m_end);

WebCore style is to use std::min/std::max.

r- to fix the isPointInRange thing.

[Pierre d'Herbemont]

(In reply to comment #5)
> (From update of attachment 29471 [details] [review])
> 
> >      if (m_readyState == HAVE_NOTHING || !m_player) {
> > +        printf("HTMLMediaElement::seek INVALID_STATE_ERR\n");
> 
> You have an extraneous printf here

Removed as off v4.

> > diff --git a/WebCore/html/TimeRanges.cpp b/WebCore/html/TimeRanges.cpp
> > index ad81ac8..2d69229 100644
> 
> > +    unsigned int overlappingArcIndex;
> > +    Range addedRange(start, end);
> > +
> > +    for (overlappingArcIndex = 0; overlappingArcIndex < m_ranges.size(); overlappingArcIndex++) {
> > +        if (addedRange.isOverlappingRange(m_ranges[overlappingArcIndex])) {
> > +            addedRange = addedRange.unionWithOverlappingRange(m_ranges[overlappingArcIndex]);
> > +            m_ranges.remove(overlappingArcIndex);
> > +            overlappingArcIndex--;
> > +        } else {
> > +            // Check the case for which there is no more to do
> > +            if (!overlappingArcIndex) {
> > +                if (addedRange.isBeforeRange(m_ranges[0]))
> > +                    break;
> > +            } else {
> > +                if (m_ranges[overlappingArcIndex - 1].isBeforeRange(addedRange)
> > +                    && addedRange.isBeforeRange(m_ranges[overlappingArcIndex])) {
> > +                    break;
> > +                }
> > +            }
> > +        }
> > +    }
> > +
> > +    // Now that we are sure we don't overlap with any arc, just add it.
> > +    m_ranges.insert(overlappingArcIndex, addedRange);
> 
> How confident are you that this handles all possible range configurations?
> Range math is tricky.

Here, we do fully inclusive range. All the case should be handled. Most of them are handled in the test case. Because we are doing inclusive range, corner case are less tricky.

> 
> > +        inline bool isPointInRange(float point)
> > +        {
> > +            return m_start <= point && point <= m_end;
> > +        }
> 
> I think this should be 
>   return m_start <= point && point < m_end;
> 
> so you'll also have to test for contiguous ranges in your amalgamation code.

Ok. I'll switch to semi exclusive range (from [---] to [---[ )

> 
> > +        inline bool isOverlappingRange(Range range)
> > +        {
> > +            return isPointInRange(range.m_start) || isPointInRange(range.m_end) || range.isPointInRange(m_end);
> > +        }
> 
> Are you sure this handles both types of entirely nested ranges?

Yes. Attempt a want to be clear Proof:
              [---0---]
 [----1----]  [----2----]
    [--------3-------]
       [------A-----]
------------------------->
Range A regarding the four possible overlapping ranges cases (range 0, 1, 2, 3):

Range.end is in A <=> Range overlaps with A, as would Range 1 or Range 0.
Range.start is in A <=> Range overlaps with A, as would Range 2 or Range 0.
a.end is in Range <=> Range overlaps with A, as would Range 3.

> 
> > +        inline Range unionWithOverlappingRange(Range range)
> > +        {
> > +            Range ret;
> > +
> > +            ret.m_start = fminf(m_start, range.m_start);
> > +            ret.m_end = fmaxf(m_end, range.m_end);
> 
> WebCore style is to use std::min/std::max.

oki.

> r- to fix the isPointInRange thing.

I will then.

[Pierre d'Herbemont]

Created attachment 29510 [details]
patch v5.

This, time with semi-open interval [---[ as Simon requested.

[Pierre d'Herbemont]

Created attachment 29539 [details]
patch v6.

(The fminf and fmaxf part was forgotten in previous patch. Adding it.)

[Simon Fraser (smfr)]

Comment on attachment 29539 [details]
patch v6.

> diff --git a/LayoutTests/media/video-played-expected.txt b/LayoutTests/media/video-played-expected.txt
> new file mode 100644
> index 0000000..ff13698
> --- /dev/null
> +++ b/LayoutTests/media/video-played-expected.txt
> @@ -0,0 +1,125 @@
> +FAIL: Timed out waiting for notifyDone to be called

FAIL indeed! Expected results should not have FAIL in them.

> -2009-04-15  Pierre d'Herbemont  <[e-mail]>
> [phone]  Pierre d'Herbemont  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add support the the media element 'played' attribute.

"for the"

I'd like to see a little more verbage saying what adding this support entailed.

>  PassRefPtr<TimeRanges> HTMLMediaElement::played() const
>  {
> -    // FIXME track played
> -    return TimeRanges::create();
> +    if (m_playing)
> +        m_playedTimeRanges->add(m_lastSeekTime, currentTime());
> +    return m_playedTimeRanges;
>  }

I think this needs to return a copy of the ranges, so that the pointer you hand back
is not 'live'.

> +{
> +    ASSERT(start <= end);
> +    unsigned int overlappingArcIndex;
> +    Range addedRange(start, end);
> +
> +    // For each present range check if we need to:
> +    // - merge with the added range, in case we are overlapping or contiguous
> +    // - Need to insert in place, we we are completely, not overlapping and not contiguous
> +    // in between two ranges.
> +    //
> +    // TODO: Given that we assume that ranges are correctly ordered, we could optimize the insert place.
> +
> +    for (overlappingArcIndex = 0; overlappingArcIndex < m_ranges.size(); overlappingArcIndex++) {
> +        if (addedRange.isOverlappingRange(m_ranges[overlappingArcIndex])
> +         || addedRange.isContiguousWithRange(m_ranges[overlappingArcIndex]))

Normal style is to put the operators on the end of the previous line.

> +        {

Brace should be on previous line.

> +            // We need to merge the addedRange and that range.
> +            addedRange = addedRange.unionWithOverlappingOrContiguousRange(m_ranges[overlappingArcIndex]);
> +            m_ranges.remove(overlappingArcIndex);
> +            overlappingArcIndex--;
> +        } else {
> +            // Check the case for which there is no more to do
> +            if (!overlappingArcIndex) {
> +                if (addedRange.isBeforeRange(m_ranges[0]))
> +                {

Brace style wrong here.

> +                if (m_ranges[overlappingArcIndex - 1].isBeforeRange(addedRange)
> +                    && addedRange.isBeforeRange(m_ranges[overlappingArcIndex])) {

Put && on previous line.

> diff --git a/WebCore/html/TimeRanges.h b/WebCore/html/TimeRanges.h

> +    // We consider all the Ranges to be semi-interval [------[

"to be a semi-interval", or "to be the semi-interval".

> +        inline bool isOverlappingRange(Range range)

The argument should be a const Range&

> +        inline bool isContiguousWithRange(Range range)

The argument should be a const Range&

> +        inline Range unionWithOverlappingOrContiguousRange(Range range)

The argument should be a const Range&

> +        inline bool isBeforeRange(Range range)

The argument should be a const Range&

r=me with those fixes.

[Pierre d'Herbemont]

Created attachment 29552 [details]
patch v7.

Addressed all comments but the one about operator position in "if", given that it goes against coding style guideline.

[Simon Fraser (smfr)]

Comment on attachment 29552 [details]
patch v7.



> +    if (m_playing)
> +    {

Still a style violation here.

> -    // FIXME track played
> -    return TimeRanges::create();
> +    if (m_playing)
> +    {

And here.

> +        float time = currentTime();
> +        if (m_lastSeekTime < time)
> +            m_playedTimeRanges->add(m_lastSeekTime, time);
> +    }
> +    return m_playedTimeRanges->copy();
>  }
>  
>  PassRefPtr<TimeRanges> HTMLMediaElement::seekable() const
> @@ -1318,9 +1333,14 @@ void HTMLMediaElement::updatePlayState()
>          m_player->setRate(m_playbackRate);
>          m_player->play();
>          startPlaybackProgressTimer();
> +        m_playing = true;
>      } else if (!shouldBePlaying && !playerPaused) {
>          m_player->pause();
>          m_playbackProgressTimer.stop();
> +        m_playing = false;
> +        float time = currentTime();
> +        if (m_lastSeekTime < time)
> +            m_playedTimeRanges->add(m_lastSeekTime, time);
>      }
>      
>      if (renderer())
> diff --git a/WebCore/html/HTMLMediaElement.h b/WebCore/html/HTMLMediaElement.h
> index 43831b3..e16ec02 100644
> --- a/WebCore/html/HTMLMediaElement.h
> +++ b/WebCore/html/HTMLMediaElement.h
> @@ -218,6 +218,7 @@ protected:
>      Timer<HTMLMediaElement> m_progressEventTimer;
>      Timer<HTMLMediaElement> m_playbackProgressTimer;
>      Vector<RefPtr<Event> > m_pendingEvents;
> +    RefPtr<TimeRanges> m_playedTimeRanges;
>      
>      float m_playbackRate;
>      float m_defaultPlaybackRate;
> @@ -228,7 +229,7 @@ protected:
>      RefPtr<MediaError> m_error;
>  
>      float m_volume;
> -    float m_currentTimeDuringSeek;
> +    float m_lastSeekTime;
>      
>      unsigned m_previousProgress;
>      double m_previousProgressTime;
> @@ -248,6 +249,8 @@ protected:
>  
>      BehaviorRestrictions m_restrictions;
>  
> +    bool m_playing;
> +
>      // counter incremented while processing a callback from the media player, so we can avoid
>      //  calling the media engine recursively
>      int m_processingMediaPlayerCallback;
> diff --git a/WebCore/html/TimeRanges.cpp b/WebCore/html/TimeRanges.cpp
> index ad81ac8..04a8fc6 100644
> --- a/WebCore/html/TimeRanges.cpp
> +++ b/WebCore/html/TimeRanges.cpp
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2007 Apple Inc.  All rights reserved.
> + * Copyright (C) 2007, 2009 Apple Inc.  All rights reserved.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions
> @@ -34,6 +34,17 @@ TimeRanges::TimeRanges(float start, float end)
>      add(start, end);
>  }
>  
> +PassRefPtr<TimeRanges> TimeRanges::copy()
> +{
> +    RefPtr<TimeRanges> newSession = TimeRanges::create();
> +    
> +    unsigned size = m_ranges.size();
> +    for (unsigned i = 0; i < size; i++)
> +        newSession->add(m_ranges[i].m_start, m_ranges[i].m_end);
> +    
> +    return newSession.release();
> +}
> +
>  float TimeRanges::start(unsigned index, ExceptionCode& ec) const 
>  { 
>      if (index >= length()) {
> @@ -53,9 +64,46 @@ float TimeRanges::end(unsigned index, ExceptionCode& ec) const
>  }
>  
>  void TimeRanges::add(float start, float end) 
> -{ 
> -    m_ranges.append(Range(start, end));
> -    // FIXME normalize
> +{
> +    ASSERT(start <= end);
> +    unsigned int overlappingArcIndex;
> +    Range addedRange(start, end);
> +
> +    // For each present range check if we need to:
> +    // - merge with the added range, in case we are overlapping or contiguous
> +    // - Need to insert in place, we we are completely, not overlapping and not contiguous
> +    // in between two ranges.
> +    //
> +    // TODO: Given that we assume that ranges are correctly ordered, this could be optimized.
> +
> +    for (overlappingArcIndex = 0; overlappingArcIndex < m_ranges.size(); overlappingArcIndex++) {
> +        if (addedRange.isOverlappingRange(m_ranges[overlappingArcIndex])
> +         || addedRange.isContiguousWithRange(m_ranges[overlappingArcIndex])) {

> +                if (m_ranges[overlappingArcIndex - 1].isBeforeRange(addedRange)
> +                    && addedRange.isBeforeRange(m_ranges[overlappingArcIndex])) {

The indentation should be consistent. I prefer the second one.

r=me.

[Pierre d'Herbemont]

(In reply to comment #11)
> (From update of attachment 29552 [details] [review])
> 
> 
> > +    if (m_playing)
> > +    {
> 
> Still a style violation here.
> And here.

Fixed.

> > +        if (addedRange.isOverlappingRange(m_ranges[overlappingArcIndex])
> > +         || addedRange.isContiguousWithRange(m_ranges[overlappingArcIndex])) {
> 
> > +                if (m_ranges[overlappingArcIndex - 1].isBeforeRange(addedRange)
> > +                    && addedRange.isBeforeRange(m_ranges[overlappingArcIndex])) {
> 
> The indentation should be consistent. I prefer the second one.

The guidelines seems to say that first one should be preferred.

[Pierre d'Herbemont]

Created attachment 29556 [details]
patch v8.

Fixed coding style. (In the .js as well)

[Eric Carlson]

http://trac.webkit.org/changeset/42619

[Simon Fraser (smfr)]

Reopening to assign to Pierre.

[Simon Fraser (smfr)]

Now you can start your fixed bug count!

[Simon Fraser (smfr)]

Tests added in http://trac.webkit.org/changeset/42757