Bug 229145 - [GStreamer][EME] Try to parse XML init datas that could possibly come from MPD manifests
Summary: [GStreamer][EME] Try to parse XML init datas that could possibly come from MP...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xabier Rodríguez Calvar
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-08-16 09:13 PDT by Xabier Rodríguez Calvar
Modified: 2021-08-19 03:51 PDT (History)
18 users (show)

See Also:


Attachments
Patch (8.31 KB, patch)
2021-08-16 09:17 PDT, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (8.54 KB, patch)
2021-08-17 03:06 PDT, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (8.57 KB, patch)
2021-08-17 07:15 PDT, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch for landing (8.57 KB, patch)
2021-08-18 00:12 PDT, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xabier Rodríguez Calvar 2021-08-16 09:13:48 PDT
[GStreamer][EME] Try to parse XML init datas that could possibly come from MPD manifests
Comment 1 Xabier Rodríguez Calvar 2021-08-16 09:17:32 PDT
Created attachment 435610 [details]
Patch
Comment 2 Philippe Normand 2021-08-16 09:25:16 PDT
Comment on attachment 435610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435610&action=review

> Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.cpp:81
> +    if (g_markup_parse_context_parse(markupParseContext.get(), payload->dataAsCharPtr(), payload->size(), &error.outPtr()) && userData.pssh)

g_warning() mentioning the error message if this fails?
Comment 3 Alicia Boya García 2021-08-16 11:22:38 PDT
Comment on attachment 435610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435610&action=review

LGTM.

>> Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.cpp:81
>> +    if (g_markup_parse_context_parse(markupParseContext.get(), payload->dataAsCharPtr(), payload->size(), &error.outPtr()) && userData.pssh)
> 
> g_warning() mentioning the error message if this fails?

It is expected to fail in the case it's not an XML, right? I would emit a warning when an XML is found but no pssh was parsed though.
Comment 4 Philippe Normand 2021-08-16 11:28:40 PDT
Comment on attachment 435610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=435610&action=review

>>> Source/WebCore/platform/graphics/gstreamer/eme/GStreamerEMEUtilities.cpp:81
>>> +    if (g_markup_parse_context_parse(markupParseContext.get(), payload->dataAsCharPtr(), payload->size(), &error.outPtr()) && userData.pssh)
>> 
>> g_warning() mentioning the error message if this fails?
> 
> It is expected to fail in the case it's not an XML, right? I would emit a warning when an XML is found but no pssh was parsed though.

Then you can omit the GError, use nullptr. If we pass a GError here and the call fails, we should warn about it.
Comment 5 Xabier Rodríguez Calvar 2021-08-17 03:06:45 PDT
Created attachment 435674 [details]
Patch
Comment 6 Xabier Rodríguez Calvar 2021-08-17 07:15:42 PDT
Created attachment 435683 [details]
Patch
Comment 7 EWS 2021-08-17 23:22:00 PDT
Alicia Boya García found in /Volumes/Data/worker/Commit-Queue/build/Source/WTF/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/worker/Commit-Queue/build/Source/WTF/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment 8 Xabier Rodríguez Calvar 2021-08-18 00:12:43 PDT
Created attachment 435751 [details]
Patch for landing
Comment 9 EWS 2021-08-18 01:26:40 PDT
Committed r281183 (240628@main): <https://commits.webkit.org/240628@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435751 [details].
Comment 10 Radar WebKit Bug Importer 2021-08-18 01:27:17 PDT
<rdar://problem/82066595>
Comment 11 Michael Catanzaro 2021-08-18 09:16:42 PDT
(In reply to EWS from comment #7)
> Alicia Boya García

Huh, this must be caused by the í in your Bugzilla name. Alica, you could probably either:

 * rename yourself in contributors.json from Garcia to García
 * rename yourself in Bugzilla from García to Garcia
Comment 12 Alicia Boya García 2021-08-19 03:51:50 PDT
(In reply to Michael Catanzaro from comment #11)
> (In reply to EWS from comment #7)
> > Alicia Boya García
> 
> Huh, this must be caused by the í in your Bugzilla name. Alica, you could
> probably either:
> 
>  * rename yourself in contributors.json from Garcia to García
>  * rename yourself in Bugzilla from García to Garcia

This is not the first patch I review. This has occurred because when calvaris send a revision of the patch with the "Reviewed by Alicia" string (since a previous patch already got r+), he used the tilde, which doesn't match contributors.json (or previous strings for that matter).

There is no reason to change my entry in contributors.json at this point, especially given how WebKit's Bugzilla still struggles with non-ASCII text, just amending the "Reviewed by" string as he did is enough.