5
\$\begingroup\$

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 <SDL/include/SDL.h>
#include <queue>
#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<uint8_t> &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<Frame> frameQueue;
    std::queue<AVFrame> 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_Errors::BUFFER_INIT_FAILURE);
    }

    queue_cond = SDL_CreateCond();

    if(!queue_cond)
    {
        return int(MadLink_Buffer_Errors::BUFFER_INIT_FAILURE);
    }

    return 0;
}

int VideoBuffer::pushFrame(std::vector<uint8_t> &buffer)
{
    if(SDL_LockMutex(queue_mutex) != 0)
    {
        return int(MadLink_Buffer_Errors::MUTEX_LOCK_FAILED);
    }

    Frame frame = Frame(buffer.data(), buffer.size());

    frameQueue.push(frame);

    SDL_UnlockMutex(queue_mutex);
    SDL_CondSignal(queue_cond);

    return 0;
}

int VideoBuffer::pushPicture(AVFrame *frame)
{
    if(SDL_LockMutex(queue_mutex) != 0)
    {
        return int(MadLink_Buffer_Errors::MUTEX_LOCK_FAILED);
    }

    pictQueue.push(*frame);

    SDL_UnlockMutex(queue_mutex);

    return 0;
}

Frame VideoBuffer::getFrame()
{
    Frame frame;

    SDL_LockMutex(queue_mutex);

    frame = frameQueue.front();

    SDL_UnlockMutex(queue_mutex);

    return frame;
}

AVFrame *VideoBuffer::getPicture()
{
    AVFrame *frame;

    SDL_LockMutex(queue_mutex);

    frame = &pictQueue.front();

    SDL_UnlockMutex(queue_mutex);

    return frame;
}

int VideoBuffer::bufferWait()
{
    return SDL_CondWait(queue_cond, queue_mutex);
}

bool VideoBuffer::isPictQueueEmpty()
{
    bool empty;

    SDL_LockMutex(queue_mutex);

    empty = pictQueue.empty();

    SDL_UnlockMutex(queue_mutex);

    return empty;
}

bool VideoBuffer::isFrameQueueEmpty()
{
    bool empty;

    SDL_LockMutex(queue_mutex);

    empty = frameQueue.empty();

    SDL_UnlockMutex(queue_mutex);

    return empty;
}

void VideoBuffer::popFrame()
{
    SDL_LockMutex(queue_mutex);

    frameQueue.pop();

    SDL_UnlockMutex(queue_mutex);
}

void VideoBuffer::popPicture()
{
    SDL_LockMutex(queue_mutex);

    pictQueue.pop();

    SDL_UnlockMutex(queue_mutex);
}

Frame.h

#ifndef FRAME_H
#define FRAME_H

#if defined _WIN32 || defined __CYGWIN__

#elif __linux__
#endif

#include <stdint.h>
#include <inttypes.h>
#include <memory>
#include <iostream>

class Frame
{
public:
    Frame();
    Frame(uint8_t *pixels);
    Frame(uint8_t *pixels, int dataSize);
    ~Frame();

    void setData(uint8_t *pixels);

    uint8_t *getData() const;
    int getSize() const;

private:
    std::shared_ptr<uint8_t> data;
    int frameSize;
    int width;
    int height;
};

#endif //FRAME_H

Frame.cpp

#include "Frame.h"

Frame::Frame()
{
}

Frame::Frame(uint8_t *pixels)
{
}

Frame::Frame(uint8_t *pixels, int dataSize)
{
    frameSize = dataSize;
    data = std::shared_ptr<uint8_t>(pixels);
}

Frame::~Frame()
{
    std::cout << "calling frame destructor" << std::endl;
    data.reset();
}

uint8_t *Frame::getData() const
{
    return data.get();
}

void Frame::setData(uint8_t *pixels)
{
    //data = pixels;
}

int Frame::getSize() const
{
    return frameSize;
}

main.cpp

#include <iostream>

#include "winsock2.h"

#include <fstream>
#include <vector>
#include <queue>
#include <list>
#include <mutex>
#include <thread>
#include "Frame.h"
#include "SDL/include/SDL.h"
#include "MadLink_Proto.h"
#include "MadLink_Logger.h"
#include "VideoBuffer.h"
using namespace std;

#if defined _WIN32 || defined __CYGWIN__
#undef main
#endif

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

#define FF_REFRESH_EVENT (SDL_USEREVENT + 1)

static int decodeThread(void *arg);
static int receiveThread(void *arg);
static int displayThread(void *arg);

AVCodecParserContext *parser;

typedef struct data {
    VideoBuffer *buffer;
    AVFrame *picture;
    SwsContext* convertContext;
    AVCodec *codec;
    AVCodecContext *codecContext;
    SOCKET s;
    SOCKADDR_IN multi;
    int fromlen;
    SDL_Renderer *renderer;
    SDL_Texture *bmp = NULL;
    SDL_Rect rect;
    AVPacket packet;
} ThreadData;

int drawFrame();

static int receiveThread(void *arg)
{
    ThreadData *data = (ThreadData *)arg;
    std::vector<uint8_t> buffer;

    // This loop receives nal packets and waits till it
    // has a complete frame before it decodes.
    while (true)
    {
        MadProto_t proto;
        int result = recvfrom(data->s, (char *)&proto, sizeof(MadProto_t), 0, (struct sockaddr *)&data->multi, &data->fromlen);

        if (result < 0)
        {
            std::cout << "receive failed! error: " << WSAGetLastError() << std::endl;
            break;
        }
        else
        {
            std::cout << "receive successful, received " << result << " bytes" << std::endl;

            std::cout << "nal length: " << ntohs(proto.nal_length) << std::endl;
            uint16_t nal_length = ntohs(proto.nal_length);

            std::copy(proto.payload, proto.payload + nal_length, std::back_inserter(buffer));

            // Frame has ended, now decode and display
            if(ntohs(proto.frame_end) == 1)
            {
                std::cout << "we have a frame end" << std::endl;
                //Frame myFrame = Frame(buffer.data(), buffer.size());

                data->buffer->pushFrame(buffer);

                //SDL_LockMutex(queue_mutex);

                //frameQueue.push(myFrame);
                //frameEndCond = SDL_TRUE;

                //SDL_UnlockMutex(queue_mutex);
                //SDL_CondSignal(queue_cond);

                buffer.clear();
            }
        }
    }
    return 0;
}

static int frameQueueGet(VideoBuffer *buffer, Frame &frame)
{
    int code = 0;

    try
    {
        std::cout << "pulling data from frame queue" << std::endl;

        for(;;)
        {
            if(buffer->isFrameQueueEmpty())
            {
                buffer->bufferWait();
            }
            else
            {
                frame = buffer->getFrame();
                buffer->popFrame();
                break;
            }
        }

//        if(SDL_LockMutex(queue_mutex) != 0)
//        {
//            std::cout << "unable to lock queue mutex in frameQueueGet()" << std::endl;
//            code = 1;
//            return code;
//        }
//
//        std::cout << "frameQueue size: " << frameQueue.size() << std::endl;
//
//        for(; ;)
//        {
//            if(frameQueue.empty())
//            {
//                std::cout << "frame queue is empty!" << std::endl;
//                SDL_CondWait(queue_cond, queue_mutex);
//            }
//            else
//            {
//                frame = frameQueue.front();
//                frameQueue.pop();
//                code = 0;
//                break;
//            }
//        }
//
//        SDL_UnlockMutex(queue_mutex);
    }
    catch(std::exception &ex)
    {
        std::cout << ex.what() << std::endl;
    }

    return code;
}

static Uint32 refreshTimerCallback(Uint32 interval, void* opaque)
{
    try
    {
        SDL_Event event;
        event.type = FF_REFRESH_EVENT;
        event.user.data1 = opaque;
        event.user.data2 = NULL;
        int result = SDL_PushEvent(&event);
        //std::cout << result << std::endl;
    }
    catch(std::exception &ex)
    {
        std::cout << ex.what() << std::endl;
    }

    return interval;
}

static void schedule_refresh(VideoBuffer *buffer, int delay)
{
    SDL_AddTimer(delay, refreshTimerCallback, &buffer);
}

static int queuePicture(ThreadData *data, AVFrame *pict)
{
    int code = 0;

    try
    {
        std::cout << "inserting into frame queue" << std::endl;

        AVFrame *frameTwo = av_frame_alloc();

        int numBytes = avpicture_get_size(PIX_FMT_YUV420P, data->codecContext->coded_width, data->codecContext->height);
        uint8_t *frameTwoBuffer = (uint8_t *) av_malloc(numBytes * sizeof(uint8_t));
        avpicture_fill((AVPicture *) frameTwo, frameTwoBuffer, PIX_FMT_YUV420P, data->codecContext->coded_width,
                       data->codecContext->height);

        sws_scale(data->convertContext, (const uint8_t *const *) pict->data, pict->linesize, 0, data->codecContext->height,
                  frameTwo->data, frameTwo->linesize);

        data->buffer->pushPicture(frameTwo);

        //av_freep(frameTwoBuffer);
        av_frame_free(&frameTwo);

//        if(SDL_LockMutex(queue_mutex) != 0)
//        {
//            std::cout << "unable to lock queue mutex in frameQueueGet()" << std::endl;
//            code = -1;
//            return code;
//        }
//
//        AVFrame *frameTwo = av_frame_alloc();
//
//        int numBytes = avpicture_get_size(PIX_FMT_YUV420P, codecContext->coded_width, codecContext->height);
//        uint8_t *frameTwoBuffer = (uint8_t *) av_malloc(numBytes * sizeof(uint8_t));
//        avpicture_fill((AVPicture *) frameTwo, frameTwoBuffer, PIX_FMT_YUV420P, codecContext->coded_width,
//                       codecContext->height);
//
//        sws_scale(convertContext, (const uint8_t *const *) pict->data, pict->linesize, 0, codecContext->height,
//                  frameTwo->data, frameTwo->linesize);
//
//        pictQueue.push(*frameTwo);
//
//        //av_freep(frameTwoBuffer);
//        //av_freep(frameTwo);
//        //av_freep(frameTwoBuffer);
//
//        SDL_UnlockMutex(queue_mutex);
    }
    catch(std::exception &ex)
    {
        std::cout << ex.what() << std::endl;
    }

    return code;
}

void videoDisplay(AVFrame &frame)
{
    int code = 0;

//    if (frame == NULL)
//        return;

//    code = SDL_UpdateYUVTexture(bmp, NULL, frame.data[0], frame.linesize[0],
//                                frame.data[1], frame.linesize[1],
//                                frame.data[2], frame.linesize[2]);
//
//    if (code < 0)
//    {
//        std::cout << "updating failed! " << SDL_GetError() << std::endl;
//        return;
//    }
//
//    SDL_RenderClear(renderer);
//    SDL_RenderCopy(renderer, bmp, NULL, NULL);
//    SDL_RenderPresent(renderer);
}

void videoRefreshTimer(void *userData)
{
    try
    {
        ThreadData *data = (ThreadData *)userData;

        if(data->buffer->isPictQueueEmpty())
        {
            AVFrame *frame = data->buffer->getPicture();

            schedule_refresh(data->buffer, 80);

            int code = 0;

            if(frame == NULL)
            {
                return;
            }

            code = SDL_UpdateYUVTexture(data->bmp, NULL, frame->data[0], frame->linesize[0],
                                        frame->data[1], frame->linesize[1],
                                        frame->data[2], frame->linesize[2]);

            if(code < 0)
            {
                std::cout << "updating failed! " << SDL_GetError() << std::endl;
                return;
            }

            SDL_RenderClear(data->renderer);
            SDL_RenderCopy(data->renderer, data->bmp, NULL, NULL);
            SDL_RenderPresent(data->renderer);

            data->buffer->popPicture();

            av_frame_free(&frame);
        }
//        if(!pictQueue.empty())
//        {
//            SDL_LockMutex(queue_mutex);
//
//            AVFrame *frame = &pictQueue.front();
//            //pictQueue.pop();
//
//            //SDL_UnlockMutex(queue_mutex);
//
//            schedule_refresh(80);
//
//            int code = 0;
//
//            if(frame == NULL)
//            {
//                return;
//            }
//
//            code = SDL_UpdateYUVTexture(bmp, NULL, frame->data[0], frame->linesize[0],
//                                        frame->data[1], frame->linesize[1],
//                                        frame->data[2], frame->linesize[2]);
//
//            if(code < 0)
//            {
//                std::cout << "updating failed! " << SDL_GetError() << std::endl;
//                return;
//            }
//
//            SDL_RenderClear(renderer);
//            SDL_RenderCopy(renderer, bmp, NULL, NULL);
//            SDL_RenderPresent(renderer);
//
//            //videoDisplay(*frame);
//
//            //av_frame_free(&frame);
//
//            pictQueue.pop();
//            //delete frame;
//            SDL_UnlockMutex(queue_mutex);
//
//            //av_frame_free(&frame);
//        }
//        else
//        {
//            schedule_refresh(1);
//        }
    }
    catch(std::exception &ex)
    {
        std::cout << "exception thrown!" << std::endl;
    }
}

static int decodeThread(void *arg)
{
    ThreadData *data = (ThreadData *)arg;
    int frameFinished;
    int decodeResult;

    try
    {

        for(; ;)
        {
            data->picture = av_frame_alloc();

            Frame frame;

            std::cout << "decoding packets!" << std::endl;
            if(frameQueueGet(data->buffer, frame) < 0)
            {
                break;
            }

            std::cout << "got frame from queue!" << std::endl;
            std::cout << "frame size: " << frame.getSize() << std::endl;

            av_init_packet(&data->packet);
            data->packet.size = frame.getSize();
            data->packet.data = frame.getData();

            decodeResult = avcodec_decode_video2(data->codecContext, data->picture, &frameFinished, &data->packet);

            std::cout << "decodeResult: " << decodeResult << std::endl;
            std::cout << "frameFinished: " << frameFinished << std::endl;

            if(frameFinished && decodeResult > 0)
            {
                if(queuePicture(data, data->picture) < 0)
                {
                    break;
                }
            }

            av_free_packet(&data->packet);
            av_frame_free(&data->picture);
        }
    }
    catch(std::exception &ex)
    {
        std::cout << "exception thrown!" << std::endl;
    }
    return 0;
}

// Get the horizontal and vertical screen sizes in pixel
void GetDesktopResolution(int& horizontal, int& vertical)
{
    RECT desktop;
    // Get a handle to the desktop window
    const HWND hDesktop = GetDesktopWindow();
    // Get the size of screen to the variable desktop
    GetWindowRect(hDesktop, &desktop);
    // The top left corner will have coordinates (0,0)
    // and the bottom right corner will have coordinates
    // (horizontal, vertical)
    horizontal = desktop.right - desktop.left;
    vertical = desktop.bottom - desktop.top;
}

int main()
{
    ThreadData *data = new ThreadData;
    SDL_Event event;
    SDL_Window *window;

    WORD wVersionRequested;
    WSADATA wsaData;
    int wsaerr;

    if (SDL_Init(SDL_INIT_EVERYTHING)) {
        fprintf(stderr, "Could not initialize SDL - %s\n", SDL_GetError());
        exit(1);
    }

    // Using MAKEWORD macro, Winsock version request 2.2
    wVersionRequested = MAKEWORD(2, 2);

    wsaerr = WSAStartup(wVersionRequested, &wsaData);

    if (wsaerr != 0)
    {
        /* Tell the user that we could not find a usable */
        /* WinSock DLL.*/
        printf("The Winsock dll not found!\n");
        return 0;
    }
    else
    {
        printf("The Winsock dll found!\n");
        printf("The status: %s.\n", wsaData.szSystemStatus);
    }

    /* Confirm that the WinSock DLL supports 2.2.*/
    /* Note that if the DLL supports versions greater    */
    /* than 2.2 in addition to 2.2, it will still return */
    /* 2.2 in wVersion since that is the version we      */
    /* requested.                                        */
    if (LOBYTE(wsaData.wVersion) != 2 || HIBYTE(wsaData.wVersion) != 2)
    {
        /* Tell the user that we could not find a usable */
        /* WinSock DLL.*/
        printf("The dll do not support the Winsock version %u.%u!\n", LOBYTE(wsaData.wVersion), HIBYTE(wsaData.wVersion));
        WSACleanup();
        return 0;
    }
    else
    {
        printf("The dll supports the Winsock version %u.%u!\n", LOBYTE(wsaData.wVersion), HIBYTE(wsaData.wVersion));
        printf("The highest version this dll can support: %u.%u\n", LOBYTE(wsaData.wHighVersion), HIBYTE(wsaData.wHighVersion));
    }

    if ((data->s = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP)) == SOCKET_ERROR)
    {
        std::cout << "socket creation failed: " << WSAGetLastError() << std::endl;
        return -1;
    }

    data->multi.sin_family = AF_INET;
    data->multi.sin_port = htons(5150);
    data->multi.sin_addr.s_addr = htonl(INADDR_ANY);
    auto bindResult = ::bind(data->s, (PSOCKADDR)&data->multi, sizeof(data->multi));

    if (bindResult < 0)
    {
        std::cout << "bindResult: " << WSAGetLastError() << std::endl;
    }

    data->fromlen = sizeof(data->multi);

    av_register_all();

    int horizontal = 0;
    int vertical = 0;

    GetDesktopResolution(horizontal, vertical);

    data->codec = avcodec_find_decoder(CODEC_ID_H264);
    if (!data->codec) {
        std::cout << "codec not found" << std::endl;
        std::cin.get();
    }

    data->codecContext = avcodec_alloc_context3(data->codec);

    if(data->codec->capabilities & CODEC_CAP_TRUNCATED)
    {
        data->codecContext->flags |= CODEC_FLAG_TRUNCATED;
    }

    //codecContext->flags |= CODEC_FLAG_LOW_DELAY;

//    if(codec->capabilities & CODEC_FLAG2_CHUNKS)
//        codecContext->flags2 |= CODEC_FLAG2_CHUNKS;

    data->codecContext->width = 1920;
    data->codecContext->height = 1080;
    data->codecContext->codec_id = CODEC_ID_H264;
    //codecContext->codec_type = AVMEDIA_TYPE_VIDEO;
    data->codecContext->pix_fmt = PIX_FMT_YUV420P;

    if (avcodec_open2(data->codecContext, data->codec, NULL) < 0) {
        std::cout << "could not open codec" << std::endl;
        std::cin.get();
    }

    data->convertContext = sws_getContext(
            1920,
            1080,
            PIX_FMT_YUV420P,
            1920,
            1080,
            PIX_FMT_YUV420P,
            SWS_BICUBIC | SWS_ACCURATE_RND,
            NULL,
            NULL,
            NULL
    );

    parser = av_parser_init(CODEC_ID_H264);

    cout << "creating sdl window" << endl;

    window = SDL_CreateWindow("YUV", SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED, 1920, 1080, SDL_WINDOW_SHOWN);

    if (window == NULL)
    {
        std::cout << "window creation failed!" << SDL_GetError() << std::endl;
        std::cin.get();
    }

    data->renderer = SDL_CreateRenderer(window, -1, 0);

    if (data->renderer == NULL)
    {
        std::cout << "unable to create renderer! " << SDL_GetError() << std::endl;
        std::cin.get();
    }

    data->bmp = SDL_CreateTexture(data->renderer, SDL_PIXELFORMAT_IYUV, SDL_TEXTUREACCESS_STATIC, 1920, 1080);

    if (data->bmp == NULL)
    {
        std::cout << "texture creation failed! " << SDL_GetError() << std::endl;
        std::cin.get();
    }

    data->rect.x = 0;
    data->rect.y = 0;
    data->rect.w = 1920;
    data->rect.h = 1080;

    data->buffer = new VideoBuffer();

    SDL_Thread *receive;
    SDL_Thread *decode;

    // Create the threads, conditions and mutexes
    //queue_mutex = SDL_CreateMutex();
    //display_mutex = SDL_CreateMutex();
    //queue_cond = SDL_CreateCond();
    //display_cond = SDL_CreateCond();

    //frame_end = SDL_CreateCond();

    cout << "creating receive thread" << endl;
    receive = SDL_CreateThread(receiveThread, "ReceiveThread", data);
    decode = SDL_CreateThread(decodeThread, "Decode Thread", data);

    schedule_refresh(data->buffer, 40);

    for (;;)
    {
        SDL_PollEvent(&event);
        switch (event.type)
        {
            case SDL_QUIT:
                SDL_Quit();
                break;
            case FF_REFRESH_EVENT:
                videoRefreshTimer(event.user.data1);
                break;
        }
    }

    std::cout << "closing everything!" << std::endl;
    //av_frame_free(&picture);
    std::cin.get();
    return 0;
}
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Can I get more information about moving the buffer into the Frame class? It makes sense, I guess I am wondering how you see it. I have added a Frame object into the VideoBuffer class. Then the VideoBuffer just fills the frame object accordingly. Is that what you meant? Also I was wondering how I post for a second code review. Do I just create a new topic pointing back to this one? \$\endgroup\$ – madprogrammer2015 Dec 31 '15 at 3:34
2
\$\begingroup\$

VideoBuffer.h / VideoBuffer.cpp

I believe the only symbol you need from the libav includes is for AVFrame, which appears to be pulled in with #include "libavcodec/avcodec.h" (which includes "libavutil/frame.h"). I haven't tested it, but you should be able to remove the other two includes.

The VideoBuffer::initialize method seems like it could be eliminated and its code moved into the constructor. I don't see the method being called explicitly in the rest of your code. Also, the member variables queue_mutex and queue_cond can be initialized in the constructor:

VideoBuffer::VideoBuffer() : queue_mutex(SDL_CreateMutex()), queue_cond(SDL_CreateCond())
{}

You could throw an exception holding the MadLink_Buffer_Errors::BUFFER_INIT_FAILURE value if initialization of either value fails.

When checking the return value from SDL_LockMutex(), comparing against zero isn't necessary. These conditionals can be rewritten:

if( SDL_LockMutex(queue_mutex) )
{
    return int(MadLink_Buffer_Errors::MUTEX_LOCK_FAILED);
}

In the VideoBuffer::pushFrame method you immediately push the Frame frame local variable into the queue and don't use the local var further. Instead you should be able to do:

frameQueue.push( Frame(buffer.data(), buffer.size()) );

In the methods VideoBuffer::getFrame(), VideoBuffer::getPicture(), VideoBuffer::isPictQueueEmpty and VideoBuffer::isFrameQueueEmpty you can assign the variables upon declaration. Also, the return code from the SDL_LockMutex() function isn't checked like in the other methods.

Frame.h / Frame.cpp

The constructor Frame::Frame(uint8_t *pixels) and Frame::setData(uint8_t *pixels) are unimplemented. The member variables of Frame can be initialized in the constructors, and Frame::width and Frame::height appear to be unused.

main.cpp

You have a using namespace std; statement early on, but seem to always use the scope resolution operator (std::*) when using objects from the std namespace (I see two instances where cout and endl are referred directly). Consider removing the using namespace std; statement.

The naming of functions is a little inconsistent. Most of the functions are named with mixedCase(), but there is also a schedule_refresh() and a GetDesktopResolution().

The first part of main() uses a lot of printf and fprintf(stderr, ...) where std::cout and std::cerr can be used instead.

You should be able to name your struct data directly instead of using a typedef:

struct ThreadData {
    ...
}

Also, if you need to disable large chunks of code, the block comments can save you a lot of typing.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.