Created attachment 303612 [details] [review]
protection: implement meta transform function

Copy the GstMeta contents over to the new buffer.

Not sure if this is generally correct, what's the scenario here? What's the transform element?

I'm playing around with a fork of gst-cencdec: https://github.com/asrashley/gst-cencdec

This is how I understood the usage of the Protection API anyway:

- qtdemux/dashdemux decorate buffers with protection meta
- demuxers also send protection events downstream
- a decryptor supporting the needed cenc protection system(s) is created and added after the demuxer

The decryptor would handle the decryption of the incoming buffers and remove the gst protection meta and other no longer needed caps members and emit buffers that can be then parsed and decoded.

So I'm trying to use one decryptor implemented as a transform element... But it can't handle the protection meta because it's been removed.

So you're saying the meta has been removed from the input buffer by the base class used by the decryptor?

Yes, the default prepare_output_buffer implementation copies the read-only buffer to a new writable buffer but the metadata is not copied over because the meta has no transform_func (see gst_buffer_copy_into towards the end of the function).

To me this make sense to implement a transform function for that meta. What may not be correct, is if this meta isn't negotiated in the caps feature. As otherwise, playbin may think this is supported. I suppose this is already the case ?

So why is it not reading the meta from the input buffer then, why does it have to be transfered to the output buffer? (This is with the decryptor being basetransform derived, right?) I feel like I'm missing something..

And the decryptor should be right after the demuxer, without any elements in between.

These metas will usually contain things like clear/crypted regions and IVs and stuff so I don't know why they would need to be copied over to decrypted output buffers?

Yes the decryptor is basetransform derived, but it receives a read-only buffer and needs to do an in-place transform so a writable buffer is created from the read-only buffer, AFAIU.

And yes the decryptor is right after the demuxer. The decryptor actually needs to read the meta to perform the content decryption, AFAIK :)

Perhaps I'm missing something there, should the decryptor then have a custom prepare_output_buffer implementation that would take care of copying the meta?

(In reply to Nicolas Dufresne (stormer) from comment #7)
> To me this make sense to implement a transform function for that meta. What
> may not be correct, is if this meta isn't negotiated in the caps feature. As
> otherwise, playbin may think this is supported. I suppose this is already
> the case ?

Well the demuxer src pad caps have application/x-cenc (for instance) and playbin looks for an element able to handle those caps, so I think we're good on that side, unless I misunderstood your question :)

Doesn't structure info need to be copied or ref-counted?

(In reply to Hyunjun from comment #11)
> Doesn't structure info need to be copied or ref-counted?

Right, seems like it needs to be copied indeed :) I'll update the patch, thanks!

Created attachment 306692 [details] [review]
protection: implement meta transform function

Copy the GstMeta contents over to the new buffer.

Comment on attachment 306692 [details] [review]
protection: implement meta transform function

Is that necessarily correct? To me it seems like it should only copy the meta if it's a COPY transform and if the complete memory is copied (region == FALSE).

Otherwise it's not clear if the protectionmeta information is still useful and usable for the transformed buffer.

Created attachment 307024 [details] [review]
protection: implement meta transform function

Copy the GstMeta contents over to the new buffer.

Thanks for the reviews Hyunjun and Sebastian. Please let me know if there's another issue to fix in the patch :)

Review of attachment 307024 [details] [review]:

Looks mostly good to me, except for small code style thing... Tim, what do you think?

::: gst/gstprotection.c
@@ +98,3 @@
+          gst_structure_copy (protection_meta->info));
+    } else
+      return FALSE;

Put some {} around here :)

Seems ok - it will only copy over the meta now if the full buffer is copied, right?

Yes

Created attachment 307221 [details] [review]
protection: implement meta transform function

Copy the GstMeta contents over to the new buffer.

Thanks, pushed:

commit ee407305588e4a18cffbd8fd60bffc9b3f03fab4
Author: Philippe Normand <philn@igalia.com>
Date:   Tue May 19 18:58:11 2015 +0200

    protection: implement meta transform function
    
    Copy the GstMeta contents over to the new buffer.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=749590