[haiku-commits] Re: haiku: hrev52133 - src/apps/mediaconverter

  • From: Dario Casalinuovo <b.vitruvio@xxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Fri, 27 Jul 2018 16:33:05 +0200

Hi, I want to comment on the patch some final thoughts

diff --git a/src/apps/mediaconverter/MediaConverterApp.cpp

b/src/apps/mediaconverter/MediaConverterApp.cpp
index 5ad77053e4..4f64f13526 100644
--- a/src/apps/mediaconverter/MediaConverterApp.cpp
+++ b/src/apps/mediaconverter/MediaConverterApp.cpp
@@ -381,11 +381,11 @@ MediaConverterApp::_ConvertFile(BMediaFile* inFile,
BMediaFile* outFile,
        int32 tracks = inFile->CountTracks();
        for (int32 i = 0; i < tracks && (!outAudTrack || !outVidTrack);
i++) {
                BMediaTrack* inTrack = inFile->TrackAt(i);
-               memset(&inFormat, 0, sizeof(media_format));
+               inFormat.Clear();


This memset is not needed. Why not just a temporary object inside the
cycle? The compiler will be able to tell and stucturing the code this way
may even avoid some unwanted bugs. This is the same like declaring a
variable outside the loop, to reuse it inside the loop and reset at each
iteration.


                inTrack->EncodedFormat(&inFormat);
                if (inFormat.IsAudio() && (audioCodec != NULL)) {
                        inAudTrack = inTrack;
-                       memset(&outAudFormat, 0, sizeof(media_format));
+                       outAudFormat.Clear();


Here the same.


                        outAudFormat.type = B_MEDIA_RAW_AUDIO;
                        raf = &(outAudFormat.u.raw_audio);
                        inTrack->DecodedFormat(&outAudFormat);
@@ -416,7 +416,7 @@ MediaConverterApp::_ConvertFile(BMediaFile* inFile,
BMediaFile* outFile,
                        height = (int32)inFormat.Height();

                        // construct desired decoded video format
-                       memset(&outVidFormat, 0, sizeof(outVidFormat));
+                       outVidFormat.Clear();


Here as well. So there's no need to clear the object anyway. It seems this
is done in the hope things would get more optimized.


                        outVidFormat.type = B_MEDIA_RAW_VIDEO;
                        rvf = &(outVidFormat.u.raw_video);
                        rvf->last_active = (uint32)(height - 1);
diff --git a/src/apps/mediaconverter/MediaConverterWindow.cpp
b/src/apps/mediaconverter/MediaConverterWindow.cpp
index 7cf8336c84..3530f89284 100644
--- a/src/apps/mediaconverter/MediaConverterWindow.cpp
+++ b/src/apps/mediaconverter/MediaConverterWindow.cpp
@@ -569,7 +569,7 @@ MediaConverterWindow::BuildAudioVideoMenus()
        media_file_format* mf_format = &(ffmi->fFileFormat);

        media_format format, outfmt;
-       memset(&format, 0, sizeof(format));
+       format.Clear();


This is an example of a really superfluous memset.


        media_codec_info codec_info;
        int32 cookie = 0;
        CodecMenuItem* cmi;
@@ -622,7 +622,7 @@ MediaConverterWindow::BuildAudioVideoMenus()
        // construct a generic video format.  Some of these parameters
        // seem silly, but are needed for R4.5.x, which is more picky
        // than subsequent BeOS releases will be.
-       memset(&format, 0, sizeof(format));
+       format.Clear();


Here too.


        format.type = B_MEDIA_RAW_VIDEO;
        format.u.raw_video.last_active = (uint32)(240 - 1);
        format.u.raw_video.orientation = B_VIDEO_TOP_LEFT_RIGHT;
diff --git a/src/apps/mediaconverter/MediaFileInfo.cpp
b/src/apps/mediaconverter/MediaFileInfo.cpp
index d5aa4f2ab8..127c96c928 100644
--- a/src/apps/mediaconverter/MediaFileInfo.cpp
+++ b/src/apps/mediaconverter/MediaFileInfo.cpp
@@ -30,7 +30,7 @@ MediaFileInfo::LoadInfo(BMediaFile* file)

        BMediaTrack* track;
        media_format format;
-       memset(&format, 0, sizeof(format));
+       format.Clear();


Again.


        media_codec_info codecInfo;
        bool audioDone(false), videoDone(false);
        bigtime_t audioDuration = 0;
@@ -57,7 +57,7 @@ MediaFileInfo::LoadInfo(BMediaFile* file)
                        "{0, plural, one{# frame} other{# frames}}"));

                if (format.IsVideo()) {
-                       memset(&format, 0, sizeof(format));
+                       format.Clear();


Here again, is there any help in reusing the same object?


                        format.type = B_MEDIA_RAW_VIDEO;

                        ret = track->DecodedFormat(&format);
@@ -87,7 +87,7 @@ MediaFileInfo::LoadInfo(BMediaFile* file)
                        videoDone = true;

                } else if (format.IsAudio()) {
-                       memset(&format, 0, sizeof(format));
+                       format.Clear();
                        format.type = B_MEDIA_RAW_AUDIO;
                        ret = track->DecodedFormat(&format);
                        if (ret != B_OK)


Same situation. I will add that the clear can be done on top of the code
anyway instead of duplicating lines. But as said, I don't see any reason in
general to reuse the same object. That's why I say for me the great part of
those memsets are superfluous and pedantic.

I will also add that in general you are not going to do so hard
computations on top of media_format. It is just something that's exchanged
during connection or when accessing a code, not really like we will have a
million of media_formats processed by a pool of threads.

-- 
Saluti,
Dario

Other related posts: