Bug 133435 – [MSE] Appends of overlapping sample data do not clear existing samples properly.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[MSE] Appends of overlapping sample data do not clear existing samples properly.

[Jer Noble]

Created attachment 232348 [details]
Patch

[Darin Adler]

Comment on attachment 232348 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232348&action=review

> Source/WTF/wtf/MediaTime.cpp:380
> +    out.printf("{%lld/%d, %lf}", m_timeValue, m_timeScale, toDouble());

The format string here says that m_timeValue is long long and m_timeScale is int, but they are actually int64_t and int32_t. To make this portable you need to either cast to the built-in types or use the PRId64 and PRId32 macros <http://en.cppreference.com/w/cpp/types/integer>.

Also, there is no value in using %lf instead of just %f. Also, %f uses a pretty ugly format with lots of zeros after the decimal point.

> Source/WTF/wtf/MediaTime.h:102
> +    void dump(PrintStream& out) const;

I don’t think the name “out” here helps.

> Source/WebCore/Modules/mediasource/SampleMap.cpp:86
> +class SamplePresentationTimeIsWithinRangeComparator {
> +public:

I suggest just making this a struct instead of the class/public combination.

> Source/WebCore/Modules/mediasource/SampleMap.cpp:87
> +    bool operator()(std::pair<MediaTime, MediaTime> range, std::pair<MediaTime, RefPtr<MediaSample>> value)

For the second argument a const std::pair& would be better to avoid churning the RefPtr. Same for the SamplePresentationTimeIsInsideRangeComparator.

> Source/WebCore/Modules/mediasource/SampleMap.cpp:91
> +    bool operator()(std::pair<MediaTime, RefPtr<MediaSample>> value, std::pair<MediaTime, MediaTime> range)

For the first argument a const std::pair& would be better to avoid churning the RefPtr. Same for the SamplePresentationTimeIsInsideRangeComparator.

> Source/WebCore/Modules/mediasource/SampleMap.cpp:207
>      std::pair<MediaTime, MediaTime> range(begin, end);

I suspect we can use the new { begin, end } syntax and thus not need to make a local temporary.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1459
> +Vector<String> SourceBuffer::bufferedSamplesForTrackID(AtomicString trackID)

A shame to churn the reference count of the atomic string. Should take a const AtomicString& instead to avoid that.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1470
> +    for (auto sampleIter = trackBuffer.samples.decodeBegin(); sampleIter != trackBuffer.samples.decodeEnd(); ++sampleIter) {
> +        MediaSample* sample = sampleIter->second.get();
> +        sampleDescriptions.append(toString(*sample));
> +    }

If only the functions were named begin/end instead of decodeBegin/decodeEnd we could use a newfangled for loop for this. Good idea for future refactoring. I don’t think the local variable is needed:

    sampleDescriptions.append(toString(*sampleIter->second));

> Source/WebCore/platform/MediaSample.h:29
> +#include "FloatSize.h"

No need to include this. We can just forward-declare FloatSize in this header.

> Source/WebCore/platform/MediaSample.h:30
> +#include <wtf/Forward.h>

Are you sure this is needed for PrintStream? Doesn’t one of the other headers include Forward.h?

> Source/WebCore/platform/MediaSample.h:74
> +    virtual void dump(PrintStream& out) const = 0;

I don’t think we need the name "out" here.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:297
> +    virtual FloatSize presentationSize() const override;

Does this need to be public? I would make it and all these other overrides private.

Also, the explicit declaration and definition of the destructor is not needed and should be removed.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:301
> +    virtual void dump(PrintStream&) const override;

Same here.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:303
>  protected:

Why protected instead of private?

> Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp:57
> +    virtual FloatSize presentationSize() const override { return FloatSize(); }

This stuff should all be private, and the destructor need not be defined.

> Source/WebCore/testing/Internals.h:330
> +    Vector<String> bufferedSamplesForTrackID(SourceBuffer*, AtomicString);

Should use const AtomicString& to avoid churning the string’s reference count.

> Source/WebCore/testing/Internals.idl:282
> +    [Conditional=MEDIA_SOURCE] DOMString[] bufferedSamplesForTrackID(SourceBuffer buffer, DOMString trackID);
> +

Don’t need this extra blank line.

[Jer Noble]

Comment on attachment 232348 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232348&action=review

>> Source/WTF/wtf/MediaTime.cpp:380
>> +    out.printf("{%lld/%d, %lf}", m_timeValue, m_timeScale, toDouble());
> 
> The format string here says that m_timeValue is long long and m_timeScale is int, but they are actually int64_t and int32_t. To make this portable you need to either cast to the built-in types or use the PRId64 and PRId32 macros <http://en.cppreference.com/w/cpp/types/integer>.
> 
> Also, there is no value in using %lf instead of just %f. Also, %f uses a pretty ugly format with lots of zeros after the decimal point.

Yes, to make this work on EFL and avoid all the 64-bit vs. 32-bit printf differences, I'll modify this to just use out.print(), which has overrides for all the various types used here.

>> Source/WTF/wtf/MediaTime.h:102
>> +    void dump(PrintStream& out) const;
> 
> I don’t think the name “out” here helps.

I'll remove it.

>> Source/WebCore/Modules/mediasource/SampleMap.cpp:86
>> +public:
> 
> I suggest just making this a struct instead of the class/public combination.

Sure thing.

>> Source/WebCore/Modules/mediasource/SampleMap.cpp:87
>> +    bool operator()(std::pair<MediaTime, MediaTime> range, std::pair<MediaTime, RefPtr<MediaSample>> value)
> 
> For the second argument a const std::pair& would be better to avoid churning the RefPtr. Same for the SamplePresentationTimeIsInsideRangeComparator.

Ok. I tried just a reference and got a template error, but a const ref seems to work.

>> Source/WebCore/Modules/mediasource/SampleMap.cpp:91
>> +    bool operator()(std::pair<MediaTime, RefPtr<MediaSample>> value, std::pair<MediaTime, MediaTime> range)
> 
> For the first argument a const std::pair& would be better to avoid churning the RefPtr. Same for the SamplePresentationTimeIsInsideRangeComparator.

Ok.

>> Source/WebCore/Modules/mediasource/SampleMap.cpp:207
>>      std::pair<MediaTime, MediaTime> range(begin, end);
> 
> I suspect we can use the new { begin, end } syntax and thus not need to make a local temporary.

Looks like no.  Since equal_range is a template, it can't infer the type being passed in that field.

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1459
>> +Vector<String> SourceBuffer::bufferedSamplesForTrackID(AtomicString trackID)
> 
> A shame to churn the reference count of the atomic string. Should take a const AtomicString& instead to avoid that.

Ok.  There are a few other places in this section of code that take bare AtomicStrings. I'll clean them up in future patches.

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1470
>> +    }
> 
> If only the functions were named begin/end instead of decodeBegin/decodeEnd we could use a newfangled for loop for this. Good idea for future refactoring. I don’t think the local variable is needed:
> 
>     sampleDescriptions.append(toString(*sampleIter->second));

The deal is: a SampleMap is a two-way structure. It maps both the decodeTime and presentationTime of given samples, so there needs to be a way to differentiate between iterating in decode order and presentation order.

But I could come up with a shorthand like "trackBuffer.samples.decodeOrder.begin()" which would allow us to use the new C++11 loops.

>> Source/WebCore/platform/MediaSample.h:29
>> +#include "FloatSize.h"
> 
> No need to include this. We can just forward-declare FloatSize in this header.

I don't think we can, since presentationSize returns a bare FloatSize, not a ref or ptr.

>> Source/WebCore/platform/MediaSample.h:30
>> +#include <wtf/Forward.h>
> 
> Are you sure this is needed for PrintStream? Doesn’t one of the other headers include Forward.h?

Looks like AtomicString.h includes Forward.h, by way of StringImpl.h.  I'll remove it.

>> Source/WebCore/platform/MediaSample.h:74
>> +    virtual void dump(PrintStream& out) const = 0;
> 
> I don’t think we need the name "out" here.

I'll remove it.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:297
>> +    virtual FloatSize presentationSize() const override;
> 
> Does this need to be public? I would make it and all these other overrides private.
> 
> Also, the explicit declaration and definition of the destructor is not needed and should be removed.

I've had it beaten into my head that all classes with virtual methods must have an explicit virtual destructor.  The implicit destructor will not be marked virtual.

I'll make all the methods private.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:301
>> +    virtual void dump(PrintStream&) const override;
> 
> Same here.

Ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:303
>>  protected:
> 
> Why protected instead of private?

No subclasses of this class exist, so there's no good reason.  We can always revert to protected if some future subclass requires it.

>> Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp:57
>> +    virtual FloatSize presentationSize() const override { return FloatSize(); }
> 
> This stuff should all be private, and the destructor need not be defined.

I really don't like leaving virtual destructors off virtual classes.  Even though no subclass of this class exists, it can lead to memory leaks if someone subclasses this class in the future without adding a virtual destructor.

I'll make it all private though.

[Jer Noble]

Created attachment 232366 [details]
Patch for landing

Addressed Darin's comments, and should fix the EFL builders.

[Jer Noble]

Committed r169523: <http://trac.webkit.org/changeset/169523>