-
Notifications
You must be signed in to change notification settings - Fork 376
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
New feature web parser with pattern #950
base: Omega
Are you sure you want to change the base?
Conversation
Could you provide some examples? Preferably real world use cases. The purpose of the feature is not clear to me from the PR description. |
It is used to search for media URLs in HTML pages using a regex pattern.
Example Regex: The current issue is that I can't send requests using custom headers. Which library should I use that works on all platforms? I'm considering using CURL, but does it work on all platforms? |
Example Usage
|
How do you ensure only one URL is selected, and also ensure it is the correct one? |
Also note that new features must be tested and merged on the Piers branch before being considered for the Omega branch. |
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 light first review. I'd suggest collapsing all commits into a single one, as many of the commits change the previous one, making it difficult to review.
Also, many of the files are missing a newline at the end of the file.
|
9d67352
to
6603db2
Compare
6603db2
to
08a4ca9
Compare
Initially, the URL fetching process was in the playlist loader. However, now that it has been moved to "GetChannelStreamProperties" there should be no issues.
It has been moved to a separate commit. I merged the all commits. |
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, review round 2.
Please take note https://github.com/xbmc/xbmc/blob/master/docs/CODE_GUIDELINES.md
src/iptvsimple/utilities/Base64.cpp
Outdated
#include "Base64.h" | ||
|
||
using namespace iptvsimple; | ||
using namespace utilities; | ||
namespace | ||
{ | ||
constexpr char PADDING{'='}; | ||
constexpr std::string_view CHARACTERS{"ABCDEFGHIJKLMNOPQRSTUVWXYZ" | ||
"abcdefghijklmnopqrstuvwxyz" | ||
"0123456789+/"}; | ||
// clang-format off | ||
constexpr unsigned char BASE64_TABLE[] = { |
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 using https://github.com/azawadzki/base-n instead as a dependency? It support base64
out of the box. Then we don't need to carry this extra code.
As far as I can tell it's standard C++ so should be cross platform.
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 pulled this code from inputstream.adaptive
. I chose this to minimize the risk of errors. If base-n
is more performant, can we first adapt the changes in inputstream.adaptive
and then apply them here as well?
Reference Code Link
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 probably take a different view on this. We should try the non custom code approach in iptvsimple and if that proves to work correctly then propose that dependency change for adaptive but not the other way around.
08a4ca9
to
a0627e9
Compare
https://github.com/xbmc/xbmc/blob/master/.clang-format The |
|
||
using namespace iptvsimple::utilities; | ||
|
||
std::string WebStreamExtractor::ExtractStreamUrl(const std::string& webUrl, |
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 code explained in this link
That clang format file is for xbmc, we don't follow all the conventions in addons just some of the more prominent ones to make code easier to read. I understand that doesn't make it black and white for contributors but it's where we are currently. |
Removed Unused Methods in CurlUtils and Base64 |
Description
Implement WebStreamExtractor to support dynamic URL extraction from web sources in playlist loading. This includes:
Missing Features
Example Usage
URL must be specified in parentheses.