[webkit-reviews] review granted: [Bug 133435] [MSE] Appends of overlapping sample data do not clear existing samples properly. : [Attachment 232348] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 133435: [MSE] Appends of overlapping sample data do not clear existing
samples properly.
https://bugs.webkit.org/show_bug.cgi?id=133435

Attachment 232348: Patch
https://bugs.webkit.org/attachment.cgi?id=232348&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.m
m: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.m
m:301
> +    virtual void dump(PrintStream&) const override;

Same here.

>
Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.m
m: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.