Video streaming client

I have written a client for streaming video playback.

The client uses two threads, a receive thread and a decode thread. Events are fired off periodically for drawing a completed frame onto a SDL window. SDL mutexes is used for creating and locking shared resources.

Currently, I am trying to split things up so I created a VideoBuffer and a Frame class. The VideoBuffer class manages the complete picture queue and the frame queue. The Frame class just represents a frame of video. The frame queue holds frames waiting to be decoded. Whereas, the picture queue holds pictures waiting to be displayed.

This client uses multiple third party libraries for its function. A completely working test case isn't readily available unless I provide the server. Which I can do if it allows for a more complete code review.

VideoBuffer.h

#ifndef VIDEOBUFFER_H
#define VIDEOBUFFER_H

#include 
#include 
#include "Frame.h"
#include "MadLink_Errors.h"
#include "MadLink_Logger.h"

extern "C" {
#include "libavcodec/avcodec.h"
#include "libavformat/avformat.h"
#include "libswscale/swscale.h"
}

class VideoBuffer
{
public:
    VideoBuffer();
    ~VideoBuffer();

    int initialize();

    int pushFrame(std::vector &buffer);
    int pushPicture(AVFrame *frame);

    int bufferWait();

    Frame getFrame();
    AVFrame *getPicture();

    bool isPictQueueEmpty();
    bool isFrameQueueEmpty();

    void popFrame();
    void popPicture();

private:
    SDL_mutex *queue_mutex;
    SDL_cond *queue_cond;
    std::queue frameQueue;
    std::queue pictQueue;

};

#endif //VIDEOBUFFER_H

VideoBuffer.cpp

```
#include "VideoBuffer.h"

VideoBuffer::VideoBuffer()
{
initialize();
}

VideoBuffer::~VideoBuffer()
{
if(queue_mutex)
{
SDL_DestroyMutex(queue_mutex);
}

if(queue_cond)
{
SDL_DestroyCond(queue_cond);
}
}

int VideoBuffer::initialize()
{
queue_mutex = SDL_CreateMutex();

if(!queue_mutex)
{
return int(MadLink_Buffer_Erro

Concurrency bugs

In frameQueueGet:

if(buffer->isFrameQueueEmpty())
{
    buffer->bufferWait();
}
else
{
    frame = buffer->getFrame();
    // ...

This only works if you have a single decoder thread. If you have two or more, the emptiness of the queue could have changed between the time you checked and the time you pop (TOCTOU race). You can't release the lock between the check and the use.
Ownership bug

In the receive side of things, your receiveThread function has a vector that it keeps "forever". It passes a reference to that vector to your video buffer, that calls the Frame with that vector's data().
So Frame's constructor creates a shared pointer to the vector's internals - that is a bad idea.

If the frame is destroyed before receiveThread returns, it'll attempt to delete the vector's internals - that's bad.

If the receive function returns before all frames are destroyed, they'll end up pointing a dead memory - that's bad too.

Even if neither of those happened, all modifications to buffer in the receive function potentially invalidate all the pointers stored in frames. And you're introducing potential data races if the data isn't invalidated.

You need to copy the data out of the receive buffer and into the frame one way or another (including potentially moving the vector out of receive and into the frame - sounds like a good move to look at), and the frame should probably keep sole ownership of it, and transfer that ownership to the decoder once it gets around to processing it.

Side note: those big swaths of commented code are distracting and waste valuable screen real-estate. Delete them. Use source control to keep track of previous versions of your code, not comments.

StackExchange Code Review Q#115380, answer score: 2