-
Notifications
You must be signed in to change notification settings - Fork 14
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
Refactor MCM Data Plane SDK API + upgrade FFmpeg plugins and sample apps #213
base: main
Are you sure you want to change the base?
Conversation
03ca990
to
7c09b10
Compare
5cbfa5f
to
7c341bf
Compare
NOTE: unit tests are all passing
|
/** | ||
* APITests setup | ||
*/ | ||
void APITests_Setup() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at https://github.com/meekrosoft/fff and consider using it. This would automate a little how mocks are written and make it much easier to check things like whether the mock function has been called
* Define configuration string field sizes | ||
*/ | ||
#define MESH_SOCKET_PATH_SIZE 108 | ||
#define MESH_IP_ADDRESS_SIZE 46 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use INET6_ADDRSTRLEN
from the <netinet/in.h>
header?
help applications declare buffers of the proper size to store IPv6 addresses in string form
INET6_ADDRSTRLEN
=46. It is the length of the string form for IPv6. Max length is for IPv4 to IPv6 address mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But after rethinking this, we should consider using the value of MAX_FROM(IPv4, IPv6, FQDN)
, so always fixed to 253. Each and every Edge
and/or Cloud
environment operates on DNS
records and use FQDN
instead of IP
, Kubernetes would be the best example here.
Each element of a domain name separated by [.] is called a “label.” The maximum length of each label is 63 characters, and a full domain name can have a maximum of 253 characters. Alphanumeric characters and hyphens can be used in labels, but a domain name must not commence or end with a hyphen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
663f269
to
fafd8d5
Compare
Signed-off-by: Konstantin Ilichev <[email protected]>
Avoid exposing the previous API to the user. Make mcm_dp.h header file accessible internally by MCM only. The user should include mesh_dp.h instead to use SDK API. Update FFmpeg patches to accomodate the change. Signed-off-by: Konstantin Ilichev <[email protected]>
* Any value of the MESH_VIDEO_PIXEL_FORMAT_* constants. | ||
*/ | ||
int pixel_format; | ||
#define MESH_VIDEO_PIXEL_FORMAT_NV12 0 ///< planar YUV 4:2:0, 12bpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you use defines instead of enums? Enums provide better type safety, and they are easier to read in debugger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to avoid introducing unnecessary type definitions for the user. Just use int
. This concept is used in the FFmpeg code base. And the other reason to adopt that approach was to make the options visible along with the structure field definition.
These macros are only used as options of the structure field definition above them. Adding an enum
will move their definitions far from the structure field, which will make it harder to understand by the user since they will need to scroll the code back and forth.
I agree, it's not an ideal approach, but I liked so much how similar structure definitions with macros bring clarity of the relationship between the field and the options in FFmpeg codebase. So I decided to adopt that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk. If I want to initialize a field and it is of a enum type this is obvious what value are possible, even IDE can help me choose one.
You right that the definition isn't exactly above, but it cannot be missed, because it directly corresponds to the field type so I can jump directly to it if I'm interested. Putting this all into struct also makes the struct itself bigger and less readable.
Another argument is that enums are less error prone when you add an argument, you cannot add the same value twice by accident.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And what about the case when the same enums/defines are used in multiple places? Should we define them twice and try to not include header file in which they were defined previously? or define just ones and lose benefit of them being defined right above the field definition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go with enum
if the compiler doesn't allow you to do the below. But it does. GCC treats enum
as an int
and prints no warnings/errors even with -Wall -Wenum_compare
.
enum MeshVideoPixelFormat {
MESH_VIDEO_PIXEL_FORMAT_NV12 = 0, ///< planar YUV 4:2:0, 12bpp
MESH_VIDEO_PIXEL_FORMAT_YUV422P = 1, ///< planar YUV 4:2:2, 16bpp
MESH_VIDEO_PIXEL_FORMAT_YUV422P10LE = 2, ///< planar YUV 4:2:2, 20bpp
MESH_VIDEO_PIXEL_FORMAT_YUV444P10LE = 3, ///< planar YUV 4:4:4, 30bpp
MESH_VIDEO_PIXEL_FORMAT_RGB8 = 4, ///< packed RGB 3:3:2, 8bpp
};
enum MeshVideoPixelFormat pixel_format;
...
pixel_format = 123;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These macro constants have a four-level namespace prefix to make them look specifically related to the MCM library. Do you think the user will be defining something like MESH_VIDEO_PIXEL_FORMAT_YUV422P10LE
in their code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh right, I forgot that only C++ prohibits this. Ok, type safety was my strongest argument. I would have chosen enum anyway, but it's mostly an opinion now. I don't like how struct filled with definitions looks, how it has broken indentation, but if you think that's better, We can do it this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I'll collect all the feedback from reviewers to decide if we go with macros or refactor to enum
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For C there are not big difference between define and enum. Moreover, MTL using define for formats and a lot of libraries doing that too (especially log libraries).
Yes, you can enable some additional checks for enum with GCC, however as @ko80 mentioned - in C there are no prohibition for that.
I would prefer to stick with define, because the same approach is used in FFMPEG, MTL, MCM
} | ||
|
||
av_log(avctx, AV_LOG_INFO, | ||
"w:%d h:%d pixfmt:%s fps:%d\n", | ||
s->width, s->height, av_get_pix_fmt_name(s->pixel_format), | ||
(int)av_q2d(s->frame_rate)); | ||
return 0; | ||
|
||
exit_delete_conn: | ||
mesh_delete_connection(&s->conn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we check the output of mesh_delete_connection here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function call is already in the path of exiting due to an error happened before. So, the extra error checking here is redundant and could potentially be misleading as to the actual error occurred in the previous place in the code. I suggest leaving it here as it is.
|
||
err = mesh_create_connection(mc, &conn); | ||
EXPECT_EQ(err, 0) << mesh_err2str(err); | ||
EXPECT_NE(conn, (MeshConnection *)NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly redundant, since if err == 0, conn cannot be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, exactly. The test is checking all the output values to be set according to expectations. Redundancy in unit tests is a must :-) You'll find more redundancy like that in the unit tests when scrolling this file down :-)
@@ -68,7 +68,8 @@ set(CPACK_PROJECT_VERSION ${PROJECT_VERSION}) | |||
include(CPack) | |||
|
|||
list(APPEND SDK_HEADERS | |||
include/mcm_dp.h | |||
# include/mcm_dp.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confused that mcm_dp.h
is no longer exposed to the user? Yeah, now the only public API is in mesh_dp.h
. And I wouldn't leave a chance to expose it again :) Let's remove that line.
pthread_mutex_destroy(&mc_ctx->mx); | ||
|
||
/** | ||
* TODO: Shutdown and deallocate connections here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder, it cannot be merged without deallocating the list mc_ctx->conn_list_head
and deallocating connections saved in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the user's responsibility to call mesh_connection_delete()
to deallocate the connection. The user had to do that in the previous PR, and I'm not changing this flow. This TODO
has been left here for the future to remind us to implement the automatic connections shutdown and deallocation in mesh_delete_client()
even when the user forgets to do that. So far it can't be implemented here because it requires additional mechanisms to gracefully shutdown the connection and synchronize everything. This is a future task.
The connections list itself doesn't need to be somehow deallocated when all connections are deleted. It is not needed by design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So currently we will have a memory leak if the user don't use mesh_delete_client() (edit: I mean mesh_connection_delete()
) for every connection?
Is it really that complicated to implement this feature? It seems that traversing though the list and calling something like mesh_delete_client on every node will be sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the user must call mesh_delete_client()
only once after deleting all connections with mesh_delete_connection()
.
I believe we don't want the user to call mesh_delete_client()
while the connection is still active in a parallel thread receiving or sending data. We don't want that to happen because there are no mechanisms of shutdown synchronization between the client and the connection. Before deleting a connection, the client must raise a signal to notify other threads that the connection and its resources are about to destroy. Then, every thread should react to this notification by closing the connection and decreasing the refcount. After that, when the connection refcount is decreased down to zero, the client can safely delete the connection.
This sort of a mechanism could be developed in the future as a new feature. For now, it's the user's responsibility to delete all connections before deleting the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so let's just return some error code if not all connections are deleted. ( As we agreed during our conversation :) )
mc_ctx = (MeshClientContext *)mc; | ||
|
||
conn_num = atomic_load_explicit(&mc_ctx->conn_num, memory_order_relaxed); | ||
while (conn_num < mc_ctx->config.max_conn_num) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it really worth playing with atomics if we anyway use mutex to lock mc_ctx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. The atomic counter is used here to avoid unnecessary locking of the mutex every time we check this counter. It is implemented this way for the future to allow collecting metrics without affecting the performance of MCM.
This atomic-based approach will be used to collect other types of metrics in the future (very near future).
} | ||
|
||
param->payload_args.video_args.pix_fmt = param->pix_fmt; | ||
param->payload_args.video_args.width = conn->cfg.payload.video.width; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it won't work for RDMA as it currently needs payload_args.rdma_args filled. As agreed, we can change rdma connection to take video_args, but it has to be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Could you add a suggestion in this comment on what fields you'd like to populate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here there is nothing to add, but we have to interpret those parameters differently in ProxyContext::RxStart_rdma and ProxyContext::TxStart_rdma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to review ONLY the *.md
files before my leave. As there are a few comments, I put them here as Comment
(not as Request changes
) to avoid blocking the merge further down the line.
#### Parameters | ||
* `[IN]` `mc` – Pointer to a parent mesh client. | ||
* `[OUT]` `conn` – Address of a pointer to the connection structure. | ||
* `[IN]` `cfg` – Pointer to a connection configuration structure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No parameter cfg
present for mesh_create_connection()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, my fault. Thanks for finding that :)
|
||
#### Returns | ||
0 if successful. Otherwise, returns an error. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about mesh_apply_connection_config_ancillary()
? - an ST2110-4x equivalent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's for the future. For now the API is going to expose only tested features. We'll enable ST40 as soon as we add full support and test it.
Puts the buffer to the media connection. | ||
|
||
#### Parameters | ||
* `[IN/OUT]` `buf` – Address of a pointer to a mesh buffer structure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the buffer is untouched (the data does not change under the address provided), I'd rather say it's an IN (static) parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the buffer is put back to MCM, the ownership of the buffer is passed from the user to MCM. And MCM will nullify this pointer in mesh_put_buffer()
to assure the user doesn't know it. So, this is an IN and OUT argument.
|
||
/* Send data loop */ | ||
for (i = 0; i < n; i++) { | ||
MeshBuffer *buf; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure which is more optimal. I have a feeling, it could be better if we put the buf
declaration outside of the loop and reuse it each time, instead of creating new pointer each time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, from the resulting assembly code perspective, you won't see any difference if you move the pointer declaration upper. It will even sit in the same memory address on the stack.
There is no creation
of the pointer in this block. It's just a declaration of a local variable on the stack which disappears when the execution flow exits the block. No memory leakage is possible here.
The only concern I could imagine here is the software developer's habit to declare local variables. From the C standard perspective, it is correct.
But yes, we can move it upper if you'd like.
1. Create client. | ||
1. Create connection. | ||
1. Configure connection. | ||
1. Establish Tx connection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that not be Rx, instead of Tx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, my fault. Will update it.
``` | ||
Creates a new mesh client from the given configuration structure. | ||
|
||
#### Parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is just me, but I would prefer to have this documentation directly in the corresponding .h file not in a separate .md file. Or if we really have to have the .md maybe it is good idea to copy it also to .h and have it in 2 places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, it should sit in mesh_dp.h
and I'm working on that ;-)
|
||
#### Parameters | ||
* `[OUT]` `mc` – Address of a pointer to a mesh client structure. | ||
* `[IN]` `cfg` – Pointer to a mesh client configuration structure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think out/in "prefix" can be shown also in the function declaration itself in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. This is what I'm working on.
int mesh_apply_connection_config_memif(MeshConnection *conn, | ||
MeshConfig_Memif *cfg) | ||
``` | ||
Applies the configuration to setup a single node direct connection via memif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"." at the end.
int mesh_apply_connection_config_rdma(MeshConnection *conn, | ||
MeshConfig_RDMA *cfg) | ||
``` | ||
Applies the configuration to setup an RDMA connection via Media Proxy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"." at the end of sentence.
int mesh_apply_connection_config_st2110(MeshConnection *conn, | ||
MeshConfig_ST2110 *cfg) | ||
``` | ||
Applies the configuration to setup an SMPTE ST2110-xx connection via Media Proxy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"." at the end of sentence.
int mesh_apply_connection_config_video(MeshConnection *conn, | ||
MeshConfig_Video *cfg) | ||
``` | ||
Applies the configuration to setup the connection payload for Video frames |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"." at the end of sentence.
int mesh_apply_connection_config_audio(MeshConnection *conn, | ||
MeshConfig_Audio *cfg) | ||
``` | ||
Applies the configuration to setup the connection payload for Audio packets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"." at the end of sentence.
Gets a text description of the error code. | ||
|
||
#### Parameters | ||
* `[IN]` `err` – Error code returned from any Mesh DP API call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is "DP"? "Data Plane" ? Define this abbreviation somewhere.
```mermaid | ||
mindmap | ||
)__MeshConnection__ configuration( | ||
**.kind** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
".kind" branch is drawn in yellow. (It is ok.) And it overwrites violet ".conn" branch. Maybe there is some way to enforce lack of overwriting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, there is no option to control the rendering process of Mermaid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'll remove the mind map from this document since it doesn't bring more clarity to the configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is quite clear. I just suggested to do it even more clear. Keep it that way.
int unsent_len; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty line not needed
|
||
strlcpy(cfg.remote_ip_addr, ip_addr, sizeof(cfg.remote_ip_addr)); | ||
|
||
if (kind == MESH_CONN_KIND_SENDER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add { / } else { / }.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assume the two options:
- Option 1
if (kind == MESH_CONN_KIND_SENDER)
cfg.remote_port = atoi(port);
else
cfg.local_port = atoi(port);
- Option 2
if (kind == MESH_CONN_KIND_SENDER) {
cfg.remote_port = atoi(port);
} else {
cfg.local_port = atoi(port);
}
Both are valid, but Option 1 is typically used when both positive and negative branches contain only one line of code. The curvy brackets are redundant here. Option 2 is mostly for cases when you have more than one line of code in the block. I'd leave this one as it is.
|
||
err = mesh_apply_connection_config_st2110(conn, &cfg); | ||
if (err) | ||
return err; | ||
} | ||
|
||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just here "return err" ?
(after of course initializing err= 0;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, what you're proposing is correct. But having this return
statement in this line clearly says that the function flow ends here. I suggest not to optimize the code prematurely.
} | ||
|
||
err = mesh_apply_connection_config_st2110(conn, &cfg); | ||
if (err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be removed if at the end of function "return err" is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above #213 (comment).
} else if (!strcmp(payload_type, "rtsp")) { | ||
param->payload_type = PAYLOAD_TYPE_RTSP_VIDEO; | ||
err = mesh_apply_connection_config_memif(conn, &cfg); | ||
if (err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be removed if at the end of function "return err" is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above #213 (comment)
cfg.transport = MESH_CONN_TRANSPORT_ST2110_30; | ||
} else { | ||
av_log(avctx, AV_LOG_ERROR, "Unknown payload type\n"); | ||
return -MESH_ERR_CONN_CONFIG_INVAL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this "return" should be remained here (even if introduced other changes with return's).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean. Could you elaborate more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not important. I commented removal of other "returns" in this function (to replace them with 1 at the end. But here there should be no change (even then).
mcm_buffer *buf = NULL; | ||
int timeout = s->first_frame ? -1 : 1000; | ||
int ret, len, err = 0; | ||
int timeout = s->first_frame ? MESH_TIMEOUT_INFINITE : 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it is worthy to pre-define this value 1000 ?
It is used 2 times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The files mcm_audio_rx.c
and mcm_audio_tx.c
define two independent FFmpeg devices. Looking from each particular device perspective, this constant is used only once. Thus, there is no actual value of replacing it with a macro. I'd leave it as it is for now. Do not optimize prematurely.
int err = 0; | ||
int ret; | ||
MeshBuffer *buf; | ||
int timeout = s->first_frame ? MESH_TIMEOUT_INFINITE : 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it is worthy to pre-define this value 1000 ?
It is used 2 times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above #213 (comment)
MeshClient *mc; | ||
MeshConnection *conn; | ||
|
||
} McmVideoMuxerContext; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should teher be whitespace before "}" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a blank line here is not prohibited by the C standard. Sometimes I add blank lines to make the structure definition look better in the editor. So, this is to make the code easier to read.
|
||
mcm_destroy_connection(s->tx_handle); | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no returning error code?
Not pushing. Just asking :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it doesn't make sense to return any error here since it's the write_trailer()
function which is called once the media stream transmission ends. After that, FFmpeg exits. So, this return value doesn't affect the flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. so maybe change of declaration to "void function_name()" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this function signature is predefined in FFmpeg. I can't change it :)
typedef struct FFOutputFormat {
...
int (*write_header)(AVFormatContext *);
/**
* Write a packet. If FF_OFMT_FLAG_ALLOW_FLUSH is set in flags_internal,
* pkt can be NULL in order to flush data buffered in the muxer.
* When flushing, return 0 if there still is more data to flush,
* or 1 if everything was flushed and there is no more buffered
* data.
*/
int (*write_packet)(AVFormatContext *, AVPacket *pkt);
int (*write_trailer)(AVFormatContext *);
...
Ready to merge? |
I'm still working on resolving discussions. |
This PR includes the following commits