GNOME Bugzilla – Bug 753613
mssdemux: PlayReady WRM parsing support
Last modified: 2015-09-30 15:57:23 UTC
It would be nice if the element could parse the ProtectionHeader node of the manifest and emit protection events so downstream element can decrypt the media. I started a patch!
Created attachment 309250 [details] [review] patch
Created attachment 309320 [details] [review] gst-indented patch
Created attachment 309444 [details] [review] updated patch Strip {} from SystemID attribute.
Created attachment 309445 [details] [review] patch gst-indent'd :)
Is there any on going work to support webm encryption ? (http://www.webmproject.org/docs/webm-encryption/) Tim that would be great if you could review Philippe's patch :)
Created attachment 310787 [details] [review] updated patch This version attempts to improve the {} stripping in the SystemID parsing, still not very happy about this part of the code, maybe I should use a regex for this?
(In reply to Julien Isorce from comment #5) > Is there any on going work to support webm encryption ? > (http://www.webmproject.org/docs/webm-encryption/) > Not to my knowledge :)
Well, the {} stripping is not elegant, but that's string ops in C for you. I don't think using a regex will improve it much. You could also do something like, i.e. strip in place. If that's less code or nicer, I don't know. It doesn't really matter much imho. gsize id_len = strlen (system_id); if (system_id[id_len - 1] == '}') { system_id[id_len - 1] = '\0'; --id_len; } /* move string one forward including the terminator */ if (system_id[0] == '{') memmove (system_id, system_id + 1, id_len - 1 + 1); Is the patch otherwise ready for review or is it still work in progress? ("started a patch" sounds like work in progress)
(In reply to Tim-Philipp Müller from comment #8) > Well, the {} stripping is not elegant, but that's string ops in C for you. I > don't think using a regex will improve it much. You could also do something > like, i.e. strip in place. If that's less code or nicer, I don't know. It > doesn't really matter much imho. > > gsize id_len = strlen (system_id); > > if (system_id[id_len - 1] == '}') { > system_id[id_len - 1] = '\0'; > --id_len; > } > > /* move string one forward including the terminator */ > if (system_id[0] == '{') > memmove (system_id, system_id + 1, id_len - 1 + 1); > Ok I can try with this. Thanks. > Is the patch otherwise ready for review or is it still work in progress? > ("started a patch" sounds like work in progress) Yes it is ready for review but I suppose we have to wait anyway because git master is frozen and this patch kind-of is a new feature rather than a critical bugfix :)
You could also use GString which does support converting to lower-case, and removing chars.
Created attachment 310802 [details] [review] mssdemux: PlayReady WRM parsing support If the manifest has a ProtectionHeader node then parse it and emit protection events according to the specified protection SystemID.
Created attachment 310873 [details] [review] updated patch
Created attachment 310874 [details] [review] updated patch Now I'm done for a while hopefully :)
Review of attachment 310874 [details] [review]: Looks good overall. Tim, as you merged the DASH CENC stuff, do you see any issues here or can we mark this as commit-after-freeze? (after this minor fix) ::: ext/smoothstreaming/gstmssdemux.c @@ +553,3 @@ + NULL); + gst_structure_set_name (s, "application/x-cenc"); + } Would you mind moving this to a function to avoid the repeated code?
Patch looks fine at first glance, didn't review in detail. The duplication of the if(protected) block you already mentioned. You could also put that fixed-size 2-element sys_ids[] array on the stack instead of alloc/freeing it there, but that's minor of course.
Created attachment 311883 [details] [review] mssdemux: PlayReady WRM parsing support If the manifest has a ProtectionHeader node then parse it and emit protection events according to the specified protection SystemID. I didn't find much code to factor out to a function apart from the caps update. The protection system selection is 2 lines only after switching to a stack allocated array.
commit ae7d938842da35f1523ea0ec3ab025327adf31c5 Author: Philippe Normand <philn@igalia.com> Date: Mon Jun 8 15:33:22 2015 +0200 mssdemux: PlayReady WRM parsing support If the manifest has a ProtectionHeader node then parse it and emit protection events according to the specified protection SystemID. https://bugzilla.gnome.org/show_bug.cgi?id=753613