Comments (11)
I sometimes write similar code to this. The point for me is that even though the if is always false now, you never know how the code will change later and suddenly the frame csrc can be invalid. This makes the code less prone to have bugs. This code could use more debug prints though to indicate that something went wrong and still work. This way it does not immediately become show stopper while alerting the dev that there is a problem.
This is just my way of coding things and I'm not sure if @altonen had the same idea or what.
I'm not sure all of the if:s are needed, but hard to say without specific list. The middle one seems unnecessary to me.
Which analysis tool did you use? Can you paste the output?
from uvgrtp.
The point for me is that even though the if is always false now, you never know how the code will change later and suddenly the frame csrc can be invalid
I expect you refer to the third example ? Not sure what you mean by "invalid". It is totally save to delete an nullptr, so there would be no need to check for it.
The only check that would make sense in my mind, would be to delete a pointer that was already deleted (that is not nullptr). But this check would not prevent it. But I might of course just miss something here.
Which analysis tool did you use? Can you paste the output?
I've used just the integration in Clion which uses Clang-tidy. I cannot paste the output, that would be way to much and not everything is really sensible. But I could embed a analyzing target into the CMake file.
I wanted to change some stuff in a PR for #13 anyway as soon as #12 is closed.
from uvgrtp.
I've been thinking about this and maybe what I meant was that checking pointers and values that come from outside the function is important, but if the value is set inside the same function then I see no sense in checking it. I'm not very familiar on what types of issues the code analyzer recognizes.
I have seen a lot of the C way, where variables are created at the start of the function, whereas I like to create them as close to their usage as possible (and initialize them for sure). This might be part of the problem and would like it changed at some point.
I would also like @altonen's opinion on the purpose of these checks.
from uvgrtp.
The first example I don't understand. I took a quick look at the code and I see that intra
given a value and flags
parameter (which the caller of uvgRTP controls) is used to determine whether late intras are dropped or not. I don't know why the analyzer says that the condition is always false, it's not at least immediately apparent after reading the code.
The second code example is a result of some refactoring, the second check for more
obviously shouldn't be there.
I have no recollection of writing that CSRC check code so I cannot comment on why the value is checked before it's released. But it can be removed of course.
from uvgrtp.
The first example I don't understand
This is because in the file h265_pkt_handler.cc
in function packet_handler
the variable intra will initially get the value INVALID_TS and never get the chance to change. So intra != INVALID_TS
can never be true.
The analyzer warns about this because this indicates that something could be wrong here. And I agree, looks strange to me to.
from uvgrtp.
@db-tech you can make a PR if you want. If there are some checks that seem like they should be there, I can add them back afterwards. It doesn't sound like there would be many if any.
Seems like this invalid intra is a separate issue.
from uvgrtp.
K, I will create a PR as soon as I find the time.
from uvgrtp.
There are a lot of function with double underscore in the name, the standard states:
the identifiers with a double underscore anywhere are reserved;
I am, personally not fond of them either :-)
Would it be ok to rename those? If so, any preferences besides just removing the underscores ?
from uvgrtp.
Sorry, I have to give up. It is just to much stuff that I cannot simply change without asking you first.
So it does not make sense for me to address those.
I would suggest that you do some static code analysis yourself and go over file by file to change the problems in one big sweep.
Because it's your code, you will be much faster then I would be.
I could offer you a pair programming session where we discuss some stuff live.
I could also build an analysis into the cmake file, then I would need an answer to this question:
#38 (comment)
from uvgrtp.
Understandable. I realized that I have been naturally running IntelliSense with msvc compilations. I also tried Clang-tidy through Qt Creator which gives slightly different results.
I have gotten a lot of warnings that I think should be addressed (I already tried addressing type conversions), but not these ones mentioned in this issue. Qt creator does offers more options for tidy that can be enabled so maybe that is the reason.
I think prefix underscores should definitely be removed. They seem to exist with local functions, so maybe an underscore afterwards or adding word local to function beginning would be a good alternative.
I'm new to this project as well, so it will take some time to establish a new style guide (with c++ stuff and more secure code base). I will create a new issue when I start working on this and will warmly welcome everyone to participate in the discussion. Some sort of meeting or two could be organized once the first draft has been formulated.
Adding analysis into CMake file would not be a bad idea.
from uvgrtp.
I removed most compiler warnings on MSVC (expect class enum) and all warnings on Linux. I also run the Clang static analyzer and fixed issues it pointed out in its report.
I'm not super experienced with static analyzers, but I fixed all warnings I could get my hands on, so I will consider this issue solved. If there is a specific tool that should be run, anyone can suggest it by opening a new issue.
from uvgrtp.
Related Issues (20)
- Multiplexing packets based on protocol HOT 3
- Streaming 4K H264 video through Wireguard VPN HOT 9
- Compilation on Nanopi board HOT 3
- RTCP interval issues HOT 2
- A mistake for APP packet payload copying HOT 1
- Python API HOT 1
- Failed to flush the message queue HOT 15
- RTP header extension HOT 2
- uint8 overflow in a test HOT 2
- H264 Failed to flush the message queue HOT 16
- H265 Failed to flush the message queue HOT 3
- Streaming H264 video HOT 2
- Visual Studio Library Linking Documentation HOT 2
- Not Receiving RTP Packets from FFmpeg HOT 5
- H26x: Aggregation causes NAL units to be sent in different order HOT 5
- H26x incorrectly detected start code if preceded by 0x1 HOT 1
- H26x 00 01 00 detected as start code at certain alignments HOT 3
- bug in reception_flow.cc HOT 3
- error in uvgrtp::formats::h26x::packet_handler HOT 2
- [Android] Library fails to compile (at least for old SDK level 21) HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from uvgrtp.