Bug 177951 – [MSE] Dead code in SourceBuffer::appendBufferTimerFired()

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Enrique Ocaña]

The second block of code here[1] is never going to be run because the first block makes the vector to be non-empty:

    size_t appendSize = m_pendingAppendData.size();
    if (!appendSize) {
        // Resize buffer for 0 byte appends so we always have a valid pointer.
        // We need to convey all appends, even 0 byte ones to |m_private| so
        // that it can clear its end of stream state if necessary.
        m_pendingAppendData.resize(1);
    }

    // Section 3.5.1 Segment Parser Loop
    // https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html#sourcebuffer-segment-parser-loop
    // When the segment parser loop algorithm is invoked, run the following steps:

    // 1. Loop Top: If the input buffer is empty, then jump to the need more data step below.
    if (!m_pendingAppendData.size()) {
        sourceBufferPrivateAppendComplete(AppendSucceeded);
        return;
    }

Probably the whole platform layer append operation could be avoided by removing the first block and just completing the append in the second code block.

Am I missing something?


[1] https://github.com/WebKit/webkit/blob/624c264648af81a0cf6a3818db9d776d4d6ee15c/Source/WebCore/Modules/mediasource/SourceBuffer.cpp#L549

[Alicia Boya García]

The comment makes me suspect that `.resize(1)` should actually be
`.reserveCapacity(1)` and that zero-length appends are not being handled
correctly.

[Enrique Ocaña]

Created attachment 322926 [details]
Patch

[Jer Noble]

This code was inherited from blink, and appears to have been totally re-written in blink since the merge.(In reply to Alicia Boya García from comment #1)
> The comment makes me suspect that `.resize(1)` should actually be
> `.reserveCapacity(1)` and that zero-length appends are not being handled
> correctly.

Seems to me that we can now change SourceBufferPrivate::append() to take a "Vector<unsigned char>&&" rather than a "void*, size_t". That would make this resize (or reserveCapacity) unnecessary, and also avoid a copy operation inside the private.

[Enrique Ocaña]

Yes, Zan wants to do that change in a different patch but he'd like to have this dead code issue sorted out before sending his patch.

[WebKit Commit Bot]

Comment on attachment 322926 [details]
Patch

Clearing flags on attachment: 322926

Committed r222965: <http://trac.webkit.org/changeset/222965>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34851791>

[Zan Dobersek]

(In reply to Enrique Ocaña from comment #4)
> Yes, Zan wants to do that change in a different patch but he'd like to have
> this dead code issue sorted out before sending his patch.

See bug #178003.