[Bug 753323] mpegts: add support for JPEG 2000 to mpegtsmux and tsdemux
GStreamer (GNOME Bugzilla)
bugzilla at gnome.org
Thu Jul 6 07:17:37 UTC 2017
https://bugzilla.gnome.org/show_bug.cgi?id=753323
Sebastian Dröge (slomo) <slomo at coaxion.net> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #353330|none |needs-work
status| |
--- Comment #99 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 353330:
--> (https://bugzilla.gnome.org/review?bug=753323&attachment=353330)
::: gst/mpegtsdemux/tsdemux.c
@@ +1494,3 @@
+ DEN_frame_rate = gst_byte_reader_get_uint16_be_unchecked (&br);
+ NUM_frame_rate = gst_byte_reader_get_uint16_be_unchecked (&br);
+ color_specification = gst_byte_reader_get_uint8_unchecked (&br);
You should check that you actually have that much data available instead of
blindly reading
@@ +1501,3 @@
+ (void) vertical_size;
+ (void) max_bit_rate;
+ (void) max_buffer_size;
G_GNUC_UNUSED seems better for that
@@ +2664,3 @@
+ GstBufferList *buffer_list = NULL;
+
+
Double empty line
@@ +2821,3 @@
+
+ buffer = gst_buffer_new_wrapped (packet_data, packet_size);
+ gst_buffer_list_add (buffer_list, buffer);
Why a buffer list? Why could there be multiple buffers here (and would each
buffer be a complete JPC frame?)?
::: gst/mpegtsmux/mpegtsmux.c
@@ +758,3 @@
+ j2k_private_data *map_data_p = NULL;
+ GstMemory *private_data =
+ gst_allocator_alloc (NULL, sizeof (j2k_private_data), NULL);
Why a GstMemory here instead of just using g_malloc()? You're not sending the
memory downstream in a buffer or similar
@@ +764,3 @@
+ GstAllocator *default_allocator =
+ gst_allocator_find (GST_ALLOCATOR_SYSMEM);
+ gst_allocator_free (default_allocator, private_data);
gst_memory_unref()
::: gst/mpegtsmux/mpegtsmux_jpeg2000.c
@@ +108,3 @@
+ guint8 seconds = (guint8) (buf->pts / 1e9);
+ guint8 minutes = (guint8) (seconds / 60);
+ guint8 hours = (guint8) (minutes / 60);
Won't these overflow the 8 bits usually? And instead of 1e9 better use
GST_SECOND
@@ +137,3 @@
+ GST_DEBUG_OBJECT (mux, "Creating buffer of size: %lu", out_size);
+
+ wr = gst_byte_writer_new ();
Initialize on the stack, maybe also with some expected size
@@ +140,3 @@
+
+ /* Elementary stream header box 'elsm' == 0x656c736d */
+ if (gst_byte_writer_put_int32_be (wr, 0x656c736d)) {
These generally can't fail, and if they do you want to go out immediately.
Please reduce the level of indentation here and below :)
@@ +278,3 @@
+ /* Free prepare data memory object */
+ GstAllocator *default_allocator = gst_allocator_find (GST_ALLOCATOR_SYSMEM);
+ gst_allocator_free (default_allocator, (GstMemory *) prepare_data);
gst_memory_unref()
::: gst/mpegtsmux/mpegtsmux_jpeg2000.h
@@ +3,3 @@
+ * Authors: Jan Schmidt <jan at fluendo.com>
+ * Kapil Agrawal <kapil at fluendo.com>
+ * Julien Moutte <julien at fluendo.com>
I don't think any of these people contributed to this new file, or the
corresponding .c file :)
::: gst/mpegtsmux/tsmux/tsmuxstream.c
@@ +798,3 @@
+ guint8 level = stream->profile_and_level & 0xF;
+ guint32 max_buffer_size = 0;
+ GstByteWriter *writer = gst_byte_writer_new ();
Initialize this on the stack ideally
--
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