Drop virtual from private member functions of Decoder class

[prabhat00155]

Only SyncDecoder class inherits Decoder class and I don't see any of these functions(marked virtual) being overwritten there.
enableLogLevel() and logCallback() functions being marked virtual is causing segfault when we cast void * to Decoder * and then call these virtual functions. Objects of classes with virtual function have a VPTR data member pointing to the VTABLE. Since, we are type-casting to Decoder * here, I think it doesn't have that information and hence throwing SIGSEGV.
For more information, check T91816044(FB internal task).

[datumbox]

Given that the classes are public, have you checked if anyone else inherits it outside of TorchVision (Github and FBcode)? What if we replace the reinterpret_cast with dynamic_cast and keep the fields virtual?

Edit:
Yeah that's not going to be a simple change:

decoder.cpp:106:17: error: 'void' is not a class type
      decoder = dynamic_cast<Decoder*>(context->opaque);
                ^                      ~~~~~~~~~~~~~~~

Dynamic casting is not going to happen if we use void* pointers all over the place. Also I don't see any other way to make the following method compile and maintaining readCallback virtual without changing the method signature and rewriting large parts of the code:

vision/torchvision/csrc/io/decoder/decoder.cpp

Lines 170 to 176 in 810811f

	int Decoder::readFunction(void* opaque, uint8_t* buf, int size) {
	Decoder* decoder = reinterpret_cast<Decoder*>(opaque);
	if (decoder == nullptr) {
	return 0;
	}
	return decoder->readCallback(buf, size);
	}

[prabhat00155]

Given that the classes are public, have you checked if anyone else inherits it outside of TorchVision (Github and FBcode)? What if we replace the reinterpret_cast with dynamic_cast and keep the fields virtual?

Edit:
Yeah that's not going to be a simple change:

decoder.cpp:106:17: error: 'void' is not a class type
      decoder = dynamic_cast<Decoder*>(context->opaque);
                ^                      ~~~~~~~~~~~~~~~

I have not checked on GitHub. Here is the diff to check tests in fbcode with this change: D28995387.

[datumbox]

@prabhat00155 I was editing my reply as you were responding... That's the 2nd time this happens to me today. 😄 Check again...

[prabhat00155]

Given that the classes are public, have you checked if anyone else inherits it outside of TorchVision (Github and FBcode)? What if we replace the reinterpret_cast with dynamic_cast and keep the fields virtual?

Edit:
Yeah that's not going to be a simple change:

decoder.cpp:106:17: error: 'void' is not a class type
      decoder = dynamic_cast<Decoder*>(context->opaque);
                ^                      ~~~~~~~~~~~~~~~

Dynamic casting is not going to happen if we use void* pointers all over the place. Also I don't see any other way to make the following method compile and maintaining readCallback virtual without changing the method signature and rewriting large parts of the code:

vision/torchvision/csrc/io/decoder/decoder.cpp

Lines 170 to 176 in 810811f

	int Decoder::readFunction(void* opaque, uint8_t* buf, int size) {
	Decoder* decoder = reinterpret_cast<Decoder*>(opaque);
	if (decoder == nullptr) {
	return 0;
	}
	return decoder->readCallback(buf, size);
	}

Yeah, opaque is a void *.

[datumbox]

LGTM, thanks!