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

Fix transcoded media ends before file is complete #1372

Merged
merged 3 commits into from
Apr 8, 2021

Conversation

dyseg
Copy link
Contributor

@dyseg dyseg commented Apr 7, 2021

Also removed ATTR_TRANSCODING_PROFILES_PROFLE_USECHUNKEDENC option, which wasn't used.

} else
ret = 0;

killAll();
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure, we do not need this kind of cleanup. I am afraid of memory leaks or dead threads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into it again. The transcoding process is always cleaned up eventually by calling ProcessIOHandler::close() either by the BufferedIOHandler class which holds the instance of ProcessIOHandler, or from the destructor. But with this modification it is only cleaned up when we transferred all content to the client, until then the transcoding process is in zombie state. For how much time it depends on the buffer settings, but likely for 10-20-30 seconds. I'll push changes to prevent this shortly.

@KarlStraussberger
Copy link
Member

Otherwise looks good to me.

@KarlStraussberger
Copy link
Member

Fixes #917

@neheb
Copy link
Contributor

neheb commented Apr 8, 2021

This breaks npupnp support. UPNP_USING_CHUNKED is not implemented.

@dyseg
Copy link
Contributor Author

dyseg commented Apr 9, 2021

This breaks npupnp support. UPNP_USING_CHUNKED is not implemented.

You're right. Should we restore the original value in case of npupnp until a solution is found? I just checked and with -1 and npupnp the transcoding is again ends before it should be. But at least this part of the code would compile.

#if defined(USING_NPUPNP)
        UpnpFileInfo_set_FileLength(info, -1);
#else
        UpnpFileInfo_set_FileLength(info, UPNP_USING_CHUNKED);
#endif

@KarlStraussberger
Copy link
Member

This breaks npupnp support. UPNP_USING_CHUNKED is not implemented.

You're right. Should we restore the original value in case of npupnp until a solution is found? I just checked and with -1 and npupnp the transcoding is again ends before it should be. But at least this part of the code would compile.

#if defined(USING_NPUPNP)
        UpnpFileInfo_set_FileLength(info, -1);
#else
        UpnpFileInfo_set_FileLength(info, UPNP_USING_CHUNKED);
#endif

Can you provide a PR, please?

@neheb
Copy link
Contributor

neheb commented Apr 9, 2021

@dyseg I posted an issue at npupnp upstream. If implemented, no. Although the minimum version should be updated.

@neheb
Copy link
Contributor

neheb commented Apr 9, 2021

Maybe just adding ifndef UPNP_USING_CHUNKED is enough.

@dyseg dyseg deleted the fix_tr branch April 12, 2021 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants