On 9/6/21 1:37 PM, Nikolay Sivov wrote:
On 9/6/21 8:23 PM, Zebediah Figura wrote:
In addition to points Zebediah made.
@@ -1211,7 +1211,8 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) source->state = SOURCE_SHUTDOWN; - unix_funcs->wg_parser_disconnect(source->wg_parser); + if (source->stream_count) + unix_funcs->wg_parser_disconnect(source->wg_parser); if (source->read_thread)
This is awkward because it implies some relationship between wg_parser being initialized and able to disconnect when stream_count is set. I think it's cleaner to let wg_parser_disconnect() handle null, or change condition to if (wg_parser).
The point here is that it's possible connection fails, in which case wg_parser is initialized but is not connected. There is no case right now where stream_count is unset but the source is connected, so I opted to use it. In the end, I will move this check into the constructor, as Zebediah suggested.
unix_funcs->wg_parser_destroy(source->wg_parser); - if (source->stream_count) + if (source->streams) free(source->streams);
This should be redundant, no?
No, it is possible for calloc to fail, in which case stream_count is set but not the array.
@@ -1231,6 +1232,9 @@ static HRESULT WINAPI media_source_Shutdown(IMFMediaSource *iface) { struct media_stream *stream = source->streams[i]; + if (!stream) + continue;
stream->state = STREAM_SHUTDOWN;
I think expected way would be to have steam_count consistent with a number of non-null streams, so you don't have to check for this.
So we should be modifying stream_count again in the failure path? I don't agree that this is more clear but I will do so.