Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UniquePtr: Support SrsUniquePtr to replace SrsAutoFree. v6.0.136 #4109

Merged
merged 8 commits into from
Jul 9, 2024

Conversation

winlinvip
Copy link
Member

@winlinvip winlinvip commented Jul 8, 2024

To manage an object:

// Before
MyClass* ptr = new MyClass();
SrsAutoFree(MyClass, ptr);
ptr->do_something();

// Now
SrsUniquePtr<MyClass> ptr(new MyClass());
ptr->do_something();

To manage an array of objects:

// Before
char* ptr = new char[10];
SrsAutoFreeA(char, ptr);
ptr[0] = 0xf;

// Now
SrsUniquePtr<char[]> ptr(new char[10]);
ptr[0] = 0xf;

In fact, SrsUniquePtr is a limited subset of SrsAutoFree, mainly managing pointers and arrays. SrsUniquePtr is better than SrsAutoFree because it has the same API to standard unique ptr.

SrsUniquePtr<MyClass> ptr(new MyClass());
ptr->do_something();
MyClass* p = ptr.get();

SrsAutoFree actually uses a pointer to a pointer, so it can be set to NULL, allowing the pointer's value to be changed later (this usage is different from SrsUniquePtr).

// OK to free ptr correctly.
MyClass* ptr;
SrsAutoFree(MyClass, ptr);
ptr = new MyClass();

// Crash because ptr is an invalid pointer.
MyClass* ptr;
SrsUniquePtr<MyClass> ptr(ptr);
ptr = new MyClass();

Additionally, SrsAutoFreeH can use specific release functions, which SrsUniquePtr does not support.


Co-authored-by: Jacob Su suzp1984@gmail.com

@winlinvip winlinvip added the EnglishNative This issue is conveyed exclusively in English. label Jul 8, 2024
@winlinvip winlinvip marked this pull request as ready for review July 8, 2024 08:55
#ifndef SRS_CORE_DEPRECATED_HPP
#define SRS_CORE_DEPRECATED_HPP

#include <srs_core.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this include header,srs_core.hpp, is useless, can be removed.

Copy link
Member Author

@winlinvip winlinvip Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to make sure the first included file is this one.

Won't fix.

// SPDX-License-Identifier: MIT
//

#include <srs_core_deprecated.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know whether it's a code conventions or not, but an empty cpp, seems useless.

Copy link
Member Author

@winlinvip winlinvip Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will cause the utest fail. Please file another PR if you want to remove it.

Won't fix.

// process all http messages.
err = process_requests(&last_req);
SrsRequest* last_req_raw = NULL;
err = process_requests(&last_req_raw);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err need to freed and processed if it's not srs_success, otherwise err != srs_success would be a memory leak.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The err is returned:

    err = process_requests(&last_req_raw);
    if ((r0 = on_disconnect(last_req.get())) != srs_success) {
        err = srs_error_wrap(err, "on disconnect %s", srs_error_desc(r0).c_str());
        srs_freep(r0);
    }
    
    return err;

Won't fix.

return srs_error_wrap(err, "create consumer, source=%s", req_->get_stream_url().c_str());
}

srs_assert(consumer);
srs_assert(consumer_raw);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consumer_raw will alway has value, no need to check with assert?

srs_error_t SrsRtcSource::create_consumer(SrsRtcConsumer*& consumer)
{
srs_error_t err = srs_success;
consumer = new SrsRtcConsumer(this);
consumers.push_back(consumer);
stream_die_at_ = 0;
// TODO: FIXME: Implements edge cluster.
return err;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a bug or logic error.

Won't fix.

pkt->wrap(msg);
} else {
// We must free it, should never use RTP packets to free it,
// because more than one RTP packet will refer to it.
SrsAutoFree(SrsRtpRawNALUs, raw);
SrsUniquePtr<SrsRtpRawNALUs> raw(raw_raw);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather use a srs_freep(raw_raw) manually in this else scope to simple the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nop, it's used later:

        SrsUniquePtr<SrsRtpRawNALUs> raw(raw_raw);
        uint8_t header = raw->skip_first_byte();

Won't fix.

return srs_error_wrap(err, "create consumer, ts source=%s", req_->get_stream_url().c_str());
}
srs_assert(consumer);

srs_assert(consumer_raw);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consumer_raw always true here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a bug or an error.

Won't fix.

Copy link
Contributor

@suzp1984 suzp1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all done

@winlinvip winlinvip changed the title UniquePtr: Support SrsUniquePtr to replace SrsAutoFree. UniquePtr: Support SrsUniquePtr to replace SrsAutoFree. v6.0.136 Jul 9, 2024
@winlinvip winlinvip added the RefinedByAI Refined by AI/GPT. label Jul 9, 2024
@winlinvip winlinvip merged commit 23d2602 into ossrs:develop Jul 9, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EnglishNative This issue is conveyed exclusively in English. RefinedByAI Refined by AI/GPT.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants