GNOME Bugzilla – Bug 743703
http connectivity robustness of hlsdemux (and now adaptivedemux)
Last modified: 2018-05-06 12:36:41 UTC
Created attachment 295768 [details] [review] hlsdemux http robustness patch We had a some issues with hlsdemux in GStreamer 1.2 with regards to http connectivity. It was with HLS playback on a commodity networking hardware (home routers, switches etc.) where connections were dropped (connection reset, etc.). At this point hlsdemux would flag an error and playback was interrupted. We've introduced a logic to hlsdemux which was responsible for figuring out what was the error exactly (required changes to souphttpsrc to pass more details on the error condition) and retrying the download if it was connectivity issue. For server errors (404 etc) we would forward the error as expected immediately. For connectivity errors we we're retrying for a fix number of attempts with a back off time and if connectivity couldn't be established (fragment couldn't be downloaded) we'd eventually surface an error. It would be fantastic it adaptivedemux could fix this problem wholesale in 1.5. I've attached our patches (for 1.2) as a reference. Hope that helps.
Created attachment 295769 [details] [review] patch souphttpsrc to expose last http error
Did you try porting your patch from 1.2 hlsdemux to master's adaptivedemux code? For next time, please update patches in git format-patch format so it integrates easily with bugzilla and git-bz. Thanks
We didn't try to port these patches to adaptivedemux as we haven't moved to 1.5 in our production code. I'm also not sure if my change to souphttpsrc is desired way of doing it. So it's more of a discussion starter.
I think this is a very good idea in general
I also support this idea and adding the new retry count and interval properties to adaptivedemux. First we need to discuss how to report errors from the http source to other elements. I'd be in favor of doing this using a GstMessage. One option is to post a GstMessage before the error message with the details of the http failure, it should have the same sequence number as the error message to identify both as being the same error reporting. The other option (which was suggested by Tim, and I'd prefer) is to extend GStreamer's Error message to support having more details about the failure.
I personally would prefer option 2 (extending GStreamer's Error message), as this seems to simplify the client code slightly.
Created attachment 296140 [details] [review] souphttpsrc: Extend GStreamer's error message. In order to include the http status code. + Port other type of errors (server, client) to this custom error.
Review of attachment 296140 [details] [review]: This is aligned with what I had in mind, but I'd prefer if we had new functions on core to set/get those details. Ideally I'd like to have a new GstStructure under the event's structure just to set these element fields, perhaps under the "element-details" name. This way we could have gst_message_set_error_element_details() and gst_message_parse_error_element_details(). OTOH, GstStructure in GstStructure is not so nice in 1.x because of serialization/deserialization limitations (use of the same symbol for opening and closing -> "") so we need to think carefully if we can do this. Even if we don't use this approach I'd still prefer to have the set/get done through some custom function in core, this way we can prevent that it overwrites standard fields used and it is easier to identify code that is using this new feature for future changes. Mathieu and Tim, what's your take on this?
Perhaps open a new bug for this against core? I also think we should API for this to GstMessage. I would like to see something extendable, so not just one kind of additional value, but the ability to send more values, so we can e.g. add the http error code, but also the URI/request that triggered it, and later perhaps additional info. So some vararg function, either one that takes multiple arguments, or one that takes one, e.g. gst_message_set_error_details (msg, "http-error-code", G_TYPE_UINT, 404, "http-error-uri", G_TYPE_STRING, uri, NULL); or gst_message_add_error_detail (msg, "http-error-code", G_TYPE_UINT, 404); gst_message_add_error_detail (msg, ...); gst_message_get_n_error_details() gst_message_parse_error_detail() All a bit annoying because of bindings and GValue variants required and such, but what can you do? (Unless we only support strings as arguments).
(In reply to comment #8) > Review of attachment 296140 [details] [review]: > > This is aligned with what I had in mind, but I'd prefer if we had new functions > on core to set/get those details. > Indeed, the "problem" with the GST_ELEMENT_ERROR macro is that it creates the message and sends it immediately, that's why I duplicated it and slightly modified it in that patch, not very nice. > Ideally I'd like to have a new GstStructure under the event's structure just to > set these element fields, perhaps under the "element-details" name. This way we > could have gst_message_set_error_element_details() and > gst_message_parse_error_element_details(). OTOH, GstStructure in GstStructure > is not so nice in 1.x because of serialization/deserialization limitations (use > of the same symbol for opening and closing -> "") so we need to think carefully > if we can do this. I remember that issue, didn't we "sort of fix it" by allowing one nested structure. I imagine there can be use cases for serialization of error messages. > > Even if we don't use this approach I'd still prefer to have the set/get done > through some custom function in core, this way we can prevent that it > overwrites standard fields used and it is easier to identify code that is using > this new feature for future changes. > > Mathieu and Tim, what's your take on this? I agree that having this in core would be nice, we will need to define a correct API, I can propose something if we decide to do so. I'll comment on the bug itself separately now.
OK so after extensive testing and looking into the history of hlsdemux, I can be affirmative that the issues this patch was fixing are mostly taken care of by adaptivedemux, yay :) First off, adaptivedemux mostly uses its own wrapped http source element, with the only exception being the update of the manifest. It handles the messages emitted by souphttpsrc and correctly retries downloading a fragment until MAX_DOWNLOAD_ERROR_COUNT is reached. There are a few ways in which adaptivedemux handles errors differently: 1) The patches made a difference between actual http errors (< 100), and failed directly when error codes were > 100, adaptivedemux will retry for all errors. 2) The number of retries is hardcoded 3) The patches exposed min and max retry times, waited first for min_retry_time before retrying, then multiplied the waiting time by two until either max_retry_time was reached or the number of retries exceeded max_retries. AdaptiveDemux always waits for half the duration of the fragment to retry. I think 1) would be desirable, I'm not sure whether 2) is needed or a sane dafault is enough, and I'm not sure either about 3), the current approach in adaptivedemux seems a bit ad hoc, maybe the heuristic in these patches is better? What do you think thiago? Side note, I have had some trouble understanding against which exact moment in the history the uridownloader patch was made : We can see here : https://github.com/ford-prefect/gst-plugins-bad/commit/d445374067996b5c5fefdf03582930bd97c86762 that Sebastian introduced the "err" field in uridownloader->priv, and at the same type modified the prototype of fetch_uri and fetch_uri_with_range to add an error parameter, but this patch acts on a version that has the old prototype, but where the err field already exists, a bit confusing, it would help me to know against what this commit was made. Right now, when I use the new -validate action to override the libc's standard receive and set new errno values, I can see that the code appropriately retries, and that when the maximum number of retries is exceeded a descriptive error is surfaced, which is IMO the expected behaviour, and renders these patches mostly obsolete.
As for 1) I think that for http server errors you should definitely stop retrying. When file is not found on the server there is little chance it will become available in the meantime. You just know it's not there. So I'd argue it's a logic error. On top of that every request can cost content providers some money. And it's not uncommon for broken assets to be put out by content providers.
(In reply to comment #12) > As for 1) I think that for http server errors you should definitely stop > retrying. When file is not found on the server there is little chance it will > become available in the meantime. You just know it's not there. So I'd argue > it's a logic error. On top of that every request can cost content providers > some money. And it's not uncommon for broken assets to be put out by content > providers. Indeed, btw s/ actual http errors (< 100) / actual http errors (> 100)/ in my previous comment. I'll propose separate patches that implement these differences and let thiago decide.
Created attachment 296213 [details] [review] adaptivedemux: Fix logic in fragment_download_finish. This was preventing us from surfacing a meaningful error.
Review of attachment 296213 [details] [review]: Please push.
Review of attachment 296213 [details] [review]: commit b6f2a962b55c4eeaf034a755e77bf40d8feafb9c Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com> Date: Thu Feb 5 18:10:15 2015 +0100 adaptivedemux: Fix logic in fragment_download_finish. This was preventing us from surfacing a meaningful error. https://bugzilla.gnome.org/show_bug.cgi?id=743703 commited
Comment on attachment 295769 [details] [review] patch souphttpsrc to expose last http error This looks like it's been obsoleted by bug #763038 ? good f94c4c00 souphttpsrc: include http-status-code in error message details
Comment on attachment 296140 [details] [review] souphttpsrc: Extend GStreamer's error message. This looks like it's been obsoleted by bug #763038 ? good f94c4c00 souphttpsrc: include http-status-code in error message details
So what's the status here? How much of this is still needed? Maybe we can split this up into multiple patches? I'm not sure if we need properties for all these things, wouldn't it be better to just #define some sane defaults?
Closing this bug report as no further information has been provided. Please feel free to reopen this bug report if you can provide the information that was asked for in a previous comment. Thanks!