[webkit-reviews] review granted: [Bug 135042] [Mac] Support audioSourceProvider() in MediaPlayerPrivateAVFoundationObjC : [Attachment 235129] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 135042: [Mac] Support audioSourceProvider() in
MediaPlayerPrivateAVFoundationObjC
https://bugs.webkit.org/show_bug.cgi?id=135042

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

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=235129&action=review


r=me with a few nits.

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:65
> +    m_capacityFrames = capacityFrames;

"m_capacityFrames" is an strange name, it initially made me think it was the
total size all frames. Maybe "m_numberOfFrames", or "m_frameCount" (and change
m_numberOfChannels to m_channelCount).

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:75
> +    Byte* channelData = static_cast<Byte*>(m_buffers->data());
> +    channelData += pointersSize;

Nit: this should be done on one line.

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:114
> +	   memcpy(*buffers + destOffset, static_cast<Byte *>(src->mData) +
srcOffset, std::min<size_t>(nbytes, src->mDataByteSize - srcOffset));

Nit: lose the space between Byte and *.

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:168
> +	   // going backwards, throw everything out
> +	   setTimeBounds(startWrite, startWrite);
> +    } else if (endWrite - startFrame() <= m_capacityFrames) {
> +	   // the buffer has not yet wrapped and will not need to.
> +	   // No-op.
> +    } else {
> +	   // advance the start time past the region we are about to overwrite
> +	   uint64_t newStart = endWrite - m_capacityFrames; // one buffer of
time behind where we're writing
> +	   uint64_t newEnd = std::max(newStart, endFrame());
> +	   setTimeBounds(newStart, newEnd);
> +    }
> +
> +    // write the new frames

Nit: these comments should be converted to WebKit style - full sentence
terminated by a period.

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:170
> +    uint32_t numberOfChannels = m_numberOfChannels;

It doesn't look like the local is never modified, why not just use
m_numberOfChannels?

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:176
> +	   // we are skipping some samples, so zero the range we are skipping

Comment style.

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:198
> +    // now update the end time

Ditto.

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:199
> +    setTimeBounds(startFrame(), endWrite);

Some of the local and member variable names used in this class. A variable name
should help the reader understand what happening, but many of these don't help
me at all. 

For example, here you are call setTimeBounds with "startFrame" and "endWrite",
but setTimeBounds has parameters called "startTime" and "endTime", which it
uses to set "m_startFrame" and "m_endFrame". Please make a pass through to make
names logical  and consistent.

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:217
> +    // fail after a few tries.

Comment style.

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:218
> +    for (int i = 0; i < 8; ++i) {

Nit: this magic value should be a named constant.

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:230
> +    return CPUOverload;

Nit: error logging here would be helpful.

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:250
> +    return Ok; // success

Nit: this comment isn't helpful.

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:265
> +CARingBuffer::Error CARingBuffer::fetch(AudioBufferList* list, size_t
nFrames, uint64_t startRead)

Nit: frameCount instead of nFrames?

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:270
> +    startRead = std::max<uint64_t>(0, startRead);

Is this necessary, how can a uint64_t be less than 0?

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:278
> +    Error err = clipTimeBounds(startRead, endRead);
> +    if (err)

Nit: you can combine these lines.

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:297
> +    Byte **buffers = static_cast<Byte**>(m_buffers->data());

No space between variable and *.

> Source/WebCore/platform/audio/mac/CARingBuffer.h:44
> +	   TooMuch, // fetch start time is earlier than buffer start time and
fetch end time is later than buffer end time
> +	   CPUOverload, // the reader is unable to get enough CPU cycles to
capture a consistent snapshot of the time bounds

Comment style.

>
Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:64
> +static double kRingBufferDuration = 1;

Nit: this can be const.

>
Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:66
> +PassRefPtr<AudioSourceProviderAVFObjC>
AudioSourceProviderAVFObjC::create(AVPlayerItem* item)

AVPlayerItem is ObjC, the space should be next to the variable name.

>
Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:73
> +AudioSourceProviderAVFObjC::AudioSourceProviderAVFObjC(AVPlayerItem* item)

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:93
> +    uint64_t availableFrames = endFrame - m_readCount;
> +    if (availableFrames <= 0)

uint64_t < 0??

>
Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:105

> +    m_ringBuffer->fetch(m_list.get(), framesToProcess, m_readCount);

fetch() returns an error, why not use it?

>
Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:305

> +void AudioSourceProviderAVFObjC::process(CMItemCount numberOfFrames,
MTAudioProcessingTapFlags flags, AudioBufferList *bufferListInOut, CMItemCount
*numberFramesOut, MTAudioProcessingTapFlags *flagsOut)

Nit: " *" -> "* "