[Bug 705991] Adding support for DASH common encryption to qtdemux and dashdemux

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=705991

--- Comment #142 from A Ashley <bugzilla at ashley-family.net> ---
(In reply to Tim-Philipp Müller from comment #139)
> Comment on attachment 307010 [details] [review]
> qtdemux: add support for ISOBMFF Common Encryption
> 
> Comments / Questions:
> ---------------------
> 
> - GST_EVENT_PROTECTION: why do we parse these and add them to the system_id
>   list, but then drop the event instead of forwarding it or storing it for
>   later forwarding? If we're going to pick up the info + system_id from the
>   headers later again anyway, we may just as well just drop the event here,
> no?

The systems IDs can be defined in either the MPD, PSSH boxes, or both. qtdemux
needs to combine all of these IDs before it calls
gst_protection_select_system(). Passing the event on is a good idea, because in
some DRM systems it might contain information required by the decryption
element.

> 
> - in decorate_and_push_buffer(): when you return GST_FLOW_ERROR, the element
>   should post a proper error message on the bus, e.g. with:
> 
>      GST_ELEMENT_ERROR (demux, STREAM, DEMUX, (NULL),
>          ("stream protected using cenc, but no cenc sample information has
> been found"));

Will do.

> 
> - parse_saiz(): if you're going to ignore info_type + info_type_parameter
>   anyway, drop it from the function and don't parse it out. If anyone needs
>   it later, they can add that later, just add a comment that you're skipping
>   those two there.
> 

Ok.

> - parse_saiz(): I think you should just return a guint8 * for the info_sizes
>   there, and fill it with sample_count times default_size if they are all
>   default_size. Should make the code a bit easier everywhere, and you save
>   the GArray which is not needed really. The extra overhead should be
>   negligible and it's temporary anyway.
> 
>   /* Parse saiz box (= the sizes of sample auxiliary info), returns array
>    * of sample_count guint8 size values, or NULL on failure */
>   static guint8 *
>   qtdemux_parse_saiz (GstQTDemux * demux, GstByteReader * br, guint32 *
> sample_count)
> 
> - general question: the CFF spec defines things such that there's an 'senc'
> box
>   and the sample aux info pointed to by 'saiz' and 'saio' is always inside
> the
>   'senc'. Isn't this always the case? If yes, we could just parse the 'senc'
>   box into a GstBuffer and attach the whole senc box to each outgoing sample,
>   plus the sample index for that sample. The decryptor can then parse out all
>   the info it needs (IV, sub sample details etc.) itself. Would save a whole
>   lot of code in qtdemux, and I doubt parsing this in the decryptor would be
>   much more code than handling all the various bits from the structure.
>   Am I missing something or am I confused, or would this be possible?
>

The problem with that approach for me is that we don't have the entire pipeline
in a single process. The inter-process communication mechanism requires
serialising the GstMeta attached to each buffer. If each sample contained the
entire senc box, it would get serialised/de-serialised for each sample, which
is a big overhead.

Also, qtdemux seems like the right place to be parsing MP4 atoms.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.