[Bug 705991] Adding support for DASH common encryption to qtdemux and dashdemux
GStreamer (GNOME Bugzilla)
bugzilla at gnome.org
Tue Jul 28 09:26:14 PDT 2015
https://bugzilla.gnome.org/show_bug.cgi?id=705991
Tim-Philipp Müller <t.i.m at zen.co.uk> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #307010|none |needs-work
status| |
--- Comment #139 from Tim-Philipp Müller <t.i.m at zen.co.uk> ---
Comment on attachment 307010
--> https://bugzilla.gnome.org/attachment.cgi?id=307010
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?
- 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"));
- 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.
- 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?
- parse_trak(): you move down the stream->caps = qtdemux_video_caps(...),
I think this breaks stuff, such as the 'if (palette_data) {' there which
adds stuff to the caps. And you moved a 'g_free (palette_data)' down which
would free data that's been used/freed in the if block earlier.
Style/Nitpicks (not so important):
----------------------------------
- #include <gst/gstprotection.h> not needed, picked up via gst/gst.h
- you generally do a lot of nesting, try more early returns/goto out, that
makes code easier to follow. Example: in configure_protected_caps(), just do:
if (stream->protection_scheme_type != FOURCC_cenc) {
GST_ERROR_OBJECT (qtdemux, "unsupported protection scheme");
return FALSE;
}
and then the main code comes next.
- G_LIKELY/UNLIKELY are probably not very useful in code paths that are
not performance critical and called rarely, but affect readability
- gst_qtdemux_append_protection_system_id(): just return from loop if found,
get rid of that existing_id boolean dance (also drop the 'else {' after
creating the array, it drops an indentation level and makes for nicer to
read code, and performance is not so much an issue here)
- protection_scheme_event_queue: embed in structure (GQueue foo_queue)
- qtdemux_uuid_bytes_to_string: just use g_strdup_printf() :)
- qtdemux_parse_pssh(): no need to check for fourcc_pssh again, since the
code that looks up the node presumably does that already...
- qtdemux_parse_pssh(): you can assume gst_event_new_*() always succeeds
- parse_cenc_aux_info(): no need to check return value of
gst_buffer_new_wrapped(), always succeeds
- decorate_and_push_buffer(): just do
while ((event = g_queue_pop_header (&foo->queue))) { ... }
and drop the empty check and the while (queue->length > 0)
- configure_protected_caps(): one could just add a NULL pointer to the
id array, and then cast (gchar **) protection_system_ids->pdata ;)
(but then if you want to add more entries later you'd have to
remove the NULL again, but aiui that's not supported anyway since
once we made a choice we've made a choice.)
--
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.
More information about the gstreamer-bugs
mailing list