Implementation of a configurable mpeg-like video traffic generator

Implementation of a configurable mpeg-like video traffic generator

The video generator module goal is to provide a flexible configurable packet generator of MPEG-4-like UDP traffic. The source code for the new module so-called (MpegPktGenClient) lives in the directory ``src/applications``. A helper module (MpegPktGenClientHelper) is also implemented to facilitate it's use. 

We can change the video streaming traffic shape, i.e. the GOP structure, by simply changing the key parameters values explained in the documentation.

The Video generator client can work with any udp based server like the already implemented in NS-3 UdpServer.

The module description, an example of usage, expected results, and a brief analysis of the output pcap trace are presented in the doc.

Acknowledgment
##############

This work is partially funded by the Inria-Alstom Transport virtual lab. The people implied in the project are (alphabetically ordered): S. Alouf, A. Dandoush, P. Derouet, P. Dersin, G. Neglia, S. Simoens, and A. Tuholukova.

[adandoush, 2016-02-12 10:58:39]

Hi,

I thank you in advance to consider the attached patch for a review process.

We have implemented a video traffic generator module. The goal is to provide a
flexible configurable packet generator of MPEG-4-like UDP traffic. The source
code for the new module so-called (MpegPktGenClient) lives in the directory
``src/applications``. A helper module (MpegPktGenClientHelper) is also
implemented to facilitate it's use. We can change the video streaming traffic
shape, i.e. the GOP, by simply changing the key parameters values explained in
the doc. The Video generator client can work with any udp based server like the
already implemented in NS-3 UdpServer. An example of usage, expected results,
and an analysis of the output trace are presented in the doc.

 Acknowledgment ############## This work is partially funded by the Inria-Alstom
Transport virtual lab. The people implied in the project are (alphabetically
ordered): S. Alouf, A. Dandoush, P. Derouet, P. Dersin, G. Neglia, S. Simoens,
and A. Tuholukova. 

Best regards,

Abdulhalim Dandoush

[Tommaso Pecorella, 2016-04-03 15:40:18]

I have a general comment. The module is useful and interesting. However its name
is kinda misleading.

A generic MPEG source can be variable bitrate or fixed bitrate, it really
depends on the kind of compression used by the video codec.
Since this model is made to closely match the data provided by one industry in
particular, it is necessary to explain exactly what kind of source it is
modelling.
As an example, one could say “This model default parameters are tuned to match a
CCD camera with xxx resolution and yyy compression kind. Tuning the parameters
you can change the resolution or the …”

This is quite important, as an MPEG flow could be originated by a surveillance
camera (like the one you used, if I’m right) or a film streaming or whatever.
Knowing our users, it’s extremely important to point out the limitations of a
model.

About the comments, I didn't comment all the ones, just the main ones. E.g.,
when I said to use Time for... times, I commented on a line but I mean all the
cases, not just one.

https://codereview.appspot.com/286160043/diff/20001/src/applications/doc/appl...
File src/applications/doc/applications.rst (right):

https://codereview.appspot.com/286160043/diff/20001/src/applications/doc/appl...
src/applications/doc/applications.rst:8: =======================
This should be Mpeg-Model, there's a spurious space.

https://codereview.appspot.com/286160043/diff/20001/src/applications/doc/appl...
src/applications/doc/applications.rst:11: 
The English should be double checked.
I'm not the right one to point out everything, as I do a lot of mistakes too,
but things like "it's use" call for a double check (it should be "its use").
The check shouldn't be only grammatical. It should consider the overall text
readability.

https://codereview.appspot.com/286160043/diff/20001/src/applications/doc/appl...
src/applications/doc/applications.rst:17: * GopLength: number of frames (I + P
or B) per Burst period. By default, its value is set to (15).
Use a format like:
* Rfc6282 (boolean, default true), used to activate HC1 (:rfc:`4944`) or IPHC
(:rfc:`6282`) compression.
* OmitUdpChecksum (boolean, default true), used to activate UDP checksum
compression in IPHC.
* FragmentReassemblyListSize (integer, default 0), indicating the number of
packets that can be reassembled at the same time. If the limit is reached, the
oldest packet is discarded. Zero means infinite.

https://codereview.appspot.com/286160043/diff/20001/src/applications/doc/appl...
src/applications/doc/applications.rst:81:
client.SetAttribute("GopLength",UintegerValue (GopLength));//def 15
Spaces after the function and before the parenthesis. Even in code examples we
should follow our syntax rules.

https://codereview.appspot.com/286160043/diff/20001/src/applications/helper/m...
File src/applications/helper/mpeg-pkt-gen-client-helper.h (right):

https://codereview.appspot.com/286160043/diff/20001/src/applications/helper/m...
src/applications/helper/mpeg-pkt-gen-client-helper.h:61: MpegPktGenClientHelper
(Ipv6Address ip, uint16_t port, std::string filename);
Document them all please.

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
File src/applications/model/mpeg-pkt-gen-client.cc (right):

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.cc:30: #include <ns3/double.h>
use a consistent bracket type

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.cc:55:
.AddConstructor<MpegPktGenClient> ()
use .SetGroupName

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.cc:67: "The maximum size of a packet
in Bytes without the headers (8 udp, 20 ip)",
IP is 20 bytes only for IPv4. I'd avoid mentioning the lower layers overhead
(the user should know) and I'd rather point out that it's the application level
packet size

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.cc:77: "Number of frames per Burst
period",
Indentations

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.cc:124: m_maxPacketSize = 1454; //
without headers (12 App, 8 udp, 20 ip)
As above.

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.cc:131: m_t0 =
Simulator::Now().GetMilliSeconds();
do *not* use milliseconds, it makes the model dependent on the time resolution.
See the comment in the header.

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.cc:136: char *videoFile)
Why char* ?

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.cc:158: //    SetVideoFile
(videoFile);
Remove commented stuff.

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.cc:257: * A longer description of the
purpose of your class after a blank
Spurious doxygen comment. Remove.

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.cc:420: if (i < entry->frameSize /
m_maxPacketSize)
If the application generates a bunch of data longer than the maximum packet
size, it should fragment them (you did it), but there's no need to pass them to
the UDP socket with some delay between them.
In a real system the application would pass them all to the UDP socket, stopping
only if the socket signals that it doesn't have any more space to buffer the
outgoing packets (back pressure).
Since we don't have (yet9 back pressure, just trust UDP.

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.cc:428: m_sendEvent =
Simulator::Schedule (MilliSeconds ( nextTime.GetMilliSeconds()*i),
&MpegPktGenClient::SendRemainPacket, this);
m_sendEvent will be overwritten in case of multiple send events, and if you have
to stop the generator this will be a huge issue.
You need a list or a better plan.

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.cc:449: m_startStopEvent =
Simulator::Schedule (MilliSeconds (nextGop), &MpegPktGenClient::ScheduleNextTx,
this);// ajouter round*1000 ms to adjust the
I don't really get this if - else.
I fear that it's possible to have a case where the simulation will stop.

In order to avoid any mistake, wouldn't it be better to store in an event the
delta time from the successive event (or a conventional value for the last one)
?
E.g., Event x --- > TimeToNextEvent 20ms. LastEvent --- > TimeToNextEvent 0ms
(zero means stop).

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.cc:496:
MpegPktGenClient::SendRemainPacket ()
There's no difference between SendRemainPacket and SendPacket

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
File src/applications/model/mpeg-pkt-gen-client.h (right):

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.h:67: * \param videoFile a path to an
auto-generated MPEG4 video file
Use a string.
Explain better.
Catch the case of null string (perhaps null could mean do *not* generate the
file?)

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.h:81: void SetRemote (Ipv6Address ip,
uint16_t port);
Document them all

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.h:93: * \return the maximum packet
size
In Bytes ?
Specify

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.h:97: * \return the Gop Length
In Bytes ?
Specify

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.h:104: uint16_t GetBitRate (void);
We have a DataRate class
(https://www.nsnam.org/docs/doxygen/classns3_1_1_data_rate.html). Please use it.

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.h:135: void ConnectionFailed
(Ptr<Socket> socket);
Documentation missing

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.h:149: double timeToSend;// diff
between the perv and curr times in ms
Use the Time class for Time quantities. Avoid double or int-as-milliseconds.

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.h:151: char frameType;// I or P
Suggestion: use an Enum

https://codereview.appspot.com/286160043/diff/20001/src/applications/model/mp...
src/applications/model/mpeg-pkt-gen-client.h:156: uint16_t m_imageRate;//Camera
imahe rate in frame/sec (fps)
what about non-integer frame rates ?