Bug 135042 – [Mac] Support audioSourceProvider() in MediaPlayerPrivateAVFoundationObjC

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[Mac] Support audioSourceProvider() in MediaPlayerPrivateAVFoundationObjC

[Jer Noble]

Created attachment 235115 [details]
Patch

[Jer Noble]

Created attachment 235129 [details]
Patch

[Eric Carlson]

Comment on attachment 235129 [details]
Patch

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: " *" -> "* "

[Jer Noble]

Created attachment 237535 [details]
Patch for landing

[Jer Noble]

Created attachment 237626 [details]
Patch for landing

[Jer Noble]

Created attachment 237707 [details]
Patch for landing

[Jer Noble]

Created attachment 237714 [details]
Patch for landing

[Jer Noble]

Created attachment 238032 [details]
Patch for landing

[Jer Noble]

Created attachment 238055 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 238055 [details]
Patch for landing

Clearing flags on attachment: 238055

Committed r173628: <http://trac.webkit.org/changeset/173628>

[Radar WebKit Bug Importer]

<rdar://problem/18838443>

[Jer Noble]

*** Bug 143332 has been marked as a duplicate of this bug. ***

[Jer Noble]

*** Bug 125031 has been marked as a duplicate of this bug. ***

[Ian]

I got to this bug from bug 135042. It was marked as a duplicate of this. 
The issue in bug 135042 does not seem resolved though.
createMediaElementSource() does not seem to work in Safari Version 8.0.5
:(

[Ian]

(In reply to comment #14)

Or in webkit nightly.

[Jer Noble]

(In reply to comment #15)
> (In reply to comment #14)
> 
> Or in webkit nightly.

You're going to have to be more specific. If you have a test case that isn't working as you expect, please file a new bug with the test case and reproduction steps.

FWIW, all my own createMediaElementSource() tests are working correctly with the latest WebKit Nightly.

[Ian]

Correction! Ok in nightly! Just an issue in Safari. My bad!