After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 748507 - mpegtsmux: set non-0 payload length in PES header if video ES packet is small enough
mpegtsmux: set non-0 payload length in PES header if video ES packet is small...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal enhancement
: 1.7.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-04-27 07:43 UTC by Hyunjun Ko
Modified: 2016-01-19 19:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mpegtsmux: set non-0 payload length in PES header if video ES packet is small enough (2.23 KB, patch)
2015-04-29 09:56 UTC, Hyunjun Ko
none Details | Review
reset pes_bytes_written when starting to write new PES packet (1.12 KB, patch)
2015-06-08 11:46 UTC, Hyunjun Ko
none Details | Review
set non-0 payload length in PES header if video ES packet is small enough (1.47 KB, patch)
2015-06-08 11:48 UTC, Hyunjun Ko
none Details | Review
mpegtsmux: set non-0 payload length in PES header if video ES packet is small enough (1.62 KB, patch)
2015-07-14 04:47 UTC, Hyunjun Ko
committed Details | Review
mpegtsmux: reset pes_bytes_written when starting to write new PES packet (1.12 KB, patch)
2015-07-14 04:48 UTC, Hyunjun Ko
committed Details | Review

Description Hyunjun Ko 2015-04-27 07:43:59 UTC
Currently, in case of video ES, it always set 0 to the length of pes.
According to ISO spec, this is right, and working fine.
But doesn't it need to set length when the length is lesser than 16bit max (in case of video ES)?

Is there any specific reason to set 0 in all video ES?
How about this logic?

if [ length is bigger than 16bit max ]
  set to 0
else 
  set to length
  
I'm using mpegtsmux in my project, and I'm thinking about this change.
Could you give me some advices for this?
Comment 1 Hyunjun Ko 2015-04-29 09:56:27 UTC
Created attachment 302549 [details] [review]
mpegtsmux: set non-0 payload length in PES header if video ES packet is small enough

Change to set non-0 video payload size if video ES packet is small enough.
Fix a bug found that pes_bytes_written is not to reset if unbounded stream.
Comment 2 Tim-Philipp Müller 2015-06-05 14:11:35 UTC
Comment on attachment 302549 [details] [review]
mpegtsmux: set non-0 payload length in PES header if video ES packet is small enough

>Fix a bug found that pes_bytes_written is not to reset if unbounded stream.

Perhaps this bug fix should be done in a separate patch?


>     /* FIXME: As a hack, for unbounded streams, start a new PES packet for each
>      * incoming packet we receive. This assumes that incoming data is 
>-     * packetised sensibly - ie, every video frame */
>-    if (stream->cur_pes_payload_size == 0)
>+     * packetised sensibly - ie, some video frames, which have length over 16 bit */

I don't think this comment needs to be changed. What the comment says is that the assumption is that the incoming stream is parsed. I don't see how your size patch changes any of that or is affected by that.


>   } else if (stream->is_video_stream) {
>-    /* Unbounded for video streams */
>-    stream->cur_pes_payload_size = 0;
>-    tsmux_stream_find_pts_dts_within (stream, stream->bytes_avail, &stream->pts,
>-        &stream->dts);
>+    if (stream->bytes_avail > G_MAXUINT16) {
>+      /* Unbounded for video streams if size is over 16 bit */
>+      stream->cur_pes_payload_size = 0;

no tsmux_stream_find_pts_dts_within() needed here?

>+    } else {
>+      stream->cur_pes_payload_size = stream->bytes_avail;
>+      tsmux_stream_find_pts_dts_within (stream, stream->cur_pes_payload_size,
>+          &stream->pts, &stream->dts);
>+    }
Comment 3 Hyunjun Ko 2015-06-08 11:38:46 UTC
> >Fix a bug found that pes_bytes_written is not to reset if unbounded stream.
> 
> Perhaps this bug fix should be done in a separate patch?
Yes. But it's not producible on current logic. Because pes_bytes_written is reset in case of bounded stream, while it is not needed to be reset in case of unbounded stream for all video packets.
But I think this fix is needed.

 
> I don't think this comment needs to be changed. What the comment says is
> that the assumption is that the incoming stream is parsed. I don't see how
> your size patch changes any of that or is affected by that.
You're right :-)


> no tsmux_stream_find_pts_dts_within() needed here?
Oops. I didn't catch it. Thanks for pointing out.
Comment 4 Hyunjun Ko 2015-06-08 11:46:33 UTC
Created attachment 304764 [details] [review]
reset pes_bytes_written when starting to write new PES packet

Split the patch into two patches.
This one is bug-fix.
Comment 5 Hyunjun Ko 2015-06-08 11:48:00 UTC
Created attachment 304765 [details] [review]
set non-0 payload length in PES header if video ES packet is small enough

Another is to set non-0 payload length patch with Tim's review.
Comment 6 Tim-Philipp Müller 2015-07-13 21:49:50 UTC
Another question: doesn't the PES packet length include some extended header bytes as well? If yes, wouldn't you want to check for something smaller than G_MAXUINT16 so you can add the header bytes without overflowing?
Comment 7 Hyunjun Ko 2015-07-14 01:37:31 UTC
(In reply to Tim-Philipp Müller from comment #6)
> Another question: doesn't the PES packet length include some extended header
> bytes as well? If yes, wouldn't you want to check for something smaller than
> G_MAXUINT16 so you can add the header bytes without overflowing?

Oh.. That's right. I didn't think of it.
It should consider header length, which is variable.
I think I have to provide new patch after thinking.
Comment 8 Hyunjun Ko 2015-07-14 04:47:46 UTC
Created attachment 307387 [details] [review]
mpegtsmux: set non-0 payload length in PES header if video ES  packet is small enough

Move comparison operation to where it can get valid pes header length.
IMO, it doesn't affect getting dts and pts.
Comment 9 Hyunjun Ko 2015-07-14 04:48:23 UTC
Created attachment 307388 [details] [review]
mpegtsmux: reset pes_bytes_written when starting to write new PES packet

Rebased on the latest git master
Comment 10 Tim-Philipp Müller 2016-01-19 19:35:47 UTC
Thanks! Sorry it took so long to get these in!

commit a274299f48799c12f5cf0e7a62ba52edcc49bbf9
Author: Hyunjun Ko <zzoon.ko@samsung.com>
Date:   Tue Jul 14 13:40:46 2015 +0900

    mpegtsmux: set non-0 payload length in PES header if video ES packet is small enough
    
    https://bugzilla.gnome.org/show_bug.cgi?id=748507

commit f9ef150652012d602fb293c999e5b2072b2da8f3
Author: Hyunjun Ko <zzoon.ko@samsung.com>
Date:   Tue Jul 14 13:42:54 2015 +0900

    mpegtsmux: reset pes_bytes_written when starting to write new PES packet
    
    In case of an unbounded packet (video usually), pes_bytes_written was
    no reset.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=748507